Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Added Composite Jobs for range events if scheduled at same instant #3752

Merged
merged 1 commit into from Jul 4, 2017

Conversation

amitjoy
Copy link
Contributor

@amitjoy amitjoy commented Jun 27, 2017

The scheduler works on the unit of seconds. For range events, it can also schedule two events to be executed at the same instant. Due to parallel execution, there are inconsistency about the executions of the range events that are scheduled for same instants.

Therefore one second to the end event has been added to assure the synchronous execution of the range start and end events.

Copy link
Contributor

@sjsf sjsf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks!

Therefore one second to the end event has been added to assure the synchronous execution of the range start and end events.

This statement just made me think - the events are executed still asynchronously in different threads, this PR does not change that. And in a multi-threaded environment we simply cannot guarantee/assure the order in which scheduled tasks are executed. We just make it more likely to happen in the right order. So maybe we should be a little more generous with respect to the padding...? What do you think?

scheduleEvent(thingUID, astroHandler, range.getEnd(), EVENT_END, channelId);

//add 1 second to the last scheduled event if both the events comprise same time instant
int secondsPadding = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I missed something, but wouldn't it be much shorter when using DateUtils directly to compare those two dates?

Calendar end = range.getEnd();
if (DateUtils.truncatedEquals(range.getStart(), end, SECOND) {
    end.add(SECOND, 1);
}

regarding the padding: either inline the value directly or declare a constant field for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I didn't know about the DateUtils#truncatedEquals method. It's more concise. will make the necessary modifications. For the constant, it is only used in one place, that's why I thought it's not a good idea to declare a constant at the interface.

@@ -39,7 +40,7 @@

/** Logger Instance */
public final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These blanks just slipped in.
Are you using the ESH IDE with the preset formatter? (Just asking to see whether it has a bug we need to fix...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have set up my IDE 5-6 days back with Eclipse Oxygen. I think I am using the latest configurations of the ESH code formatter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, so could you just manually remove the blanks then please?

@amitjoy
Copy link
Contributor Author

amitjoy commented Jun 27, 2017

This statement just made me think - the events are executed still asynchronously in different threads, this PR does not change that. And in a multi-threaded environment we simply cannot guarantee/assure the order in which scheduled tasks are executed. We just make it more likely to happen in the right order. So maybe we should be a little more generous with respect to the padding...? What do you think?

I agree it is still asynchronous. In case of range events, when START event and END event are scheduled at the same instant, the ordering of executions cannot be controlled explicitly. That's why, the ordering of the start and end event executions are controlled by delaying the end event by 1 more second. This is of course a hack to assure that the start and end events always occur one after another even though they are getting executed in different threads.

You are absolutely right. The previous 2 liner statement was kinda misleading. The events are still asynchronous. My opinion is that delaying the end event to 1 more second will not do any harm, I believe.

@sjsf
Copy link
Contributor

sjsf commented Jun 27, 2017

We are on the same page here. My question was more about whether we should increase the padding to something bigger, like 5 or even 10 seconds?

@amitjoy
Copy link
Contributor Author

amitjoy commented Jun 27, 2017

We could delay the end event to 5 or 10 more seconds but if the user needs the events to be scheduled at the same instant, then it is user specific requirement and to assure that START and END events occur one after another, we delay it to 1 sec which would have a minimal impact on what user needs. What do you think?

@sjsf
Copy link
Contributor

sjsf commented Jun 27, 2017

You are right - 1 second should work. And if it doesn't, let's agree we would need to fix it in the scheduler.

@@ -167,7 +167,7 @@ public static boolean isTimeGreaterEquals(Calendar cal1, Calendar cal2) {
Calendar truncCal2 = DateUtils.truncate(cal2, Calendar.MINUTE);
return truncCal1.getTimeInMillis() >= truncCal2.getTimeInMillis();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here two please.

@amitjoy amitjoy force-pushed the fix_range_events_delay branch 5 times, most recently from 005dae2 to 932bc5b Compare June 27, 2017 14:27
@amitjoy
Copy link
Contributor Author

amitjoy commented Jun 27, 2017

@SJKA Sure. If 1 doesn't work, we must fix it in scheduler. As you advised, I have amended my PR with the required changes.

Copy link
Contributor

@sjsf sjsf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
Let's just wait for travis to finish...

@maggu2810
Copy link
Contributor

Wait a moment before merging, I will have a look at, too.

@maggu2810
Copy link
Contributor

@amitjoy
Copy link
Contributor Author

amitjoy commented Jun 28, 2017

@maggu2810 The changes are promising. How can we proceed? I can make these necessary changes and add you as co-commiter in the PR. What do you think?

@maggu2810
Copy link
Contributor

@amitjoy Merge / cherry-pick that changes into your branch or integrate it manually.
Make it looking good. 😉
Add another Signed-off-by or leave it. I don't care for that few lines of code.

@amitjoy amitjoy force-pushed the fix_range_events_delay branch 2 times, most recently from 3707e90 to 9a9a396 Compare June 29, 2017 11:53
@amitjoy amitjoy changed the title Added delay of 1 second if range events are scheduled at same instant Added Composite Jobs for range events if scheduled at same instant Jun 29, 2017
@amitjoy
Copy link
Contributor Author

amitjoy commented Jun 29, 2017

@maggu2810 I have updated the PR according to suggestions.

Copy link
Contributor

@sjsf sjsf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This indeed is a much nicer solution than the 1 second delay!

Calendar end = range.getEnd();
if (truncatedEquals(start, end, SECOND)) {
scheduleEvent(thingUID, astroHandler, start, asList(EVENT_START, EVENT_END), channelId);
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use an if {...} else {...} block here - the early return is of course correct, but always dangerous for the next one who needs to do some archeology here (or even change it).

@amitjoy
Copy link
Contributor Author

amitjoy commented Jun 29, 2017

@SJKA I have amended the PR.

@@ -0,0 +1,47 @@
/**
* Copyright (c) 2017 by the respective copyright holders.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment below

checkArgument(!jobs.isEmpty(), "Jobs must not be empty");

this.jobs = jobs;
this.thingUID = jobs.get(0).getThingUID();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have you removed the check that every provided job is using the same thing UID?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. I might have overlooked it.

@@ -1,20 +1,26 @@
/**
* Copyright (c) 2014-2017 by the respective copyright holders.
* Copyright (c) 2017 by the respective copyright holders.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have you removed the 2014-?
Isn't the range added by the licence maven plugin that we use to update the header and to ensure every file is using the correct one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to [1] - The range in license header denotes the start year of the content creation and the current year.

[1] - https://eclipse.org/legal/copyrightandlicensenotice.php

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we would like to follow that rule we need to change the project specific Code Style guidelines and update all other source files.
IMHO this could be discussed in a separate issue.

As long as the Code Style for that project uses mvn license:format to keep the license header up to date we should follow that rule. Shouldn't we?

https://www.eclipse.org/smarthome/documentation/development/guidelines.html#a-code-style

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I will amend this PR.

@@ -66,7 +72,7 @@ public static void schedule(String thingUID, AstroThingHandler astroHandler, Job
logger.error("{}", ex.getMessage(), ex);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This trailing whitespaces indicates that your formatter is not working.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5-6 days back I have set the SmartHome environment using Eclipse Oxygen RC release using Oomph Profile. I am using the current formatter. I will try to manually remove these lines.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be wrong -- but I am pretty sure, the formatting rules and the save actions contain the removal of trailing white spaces.
Please check it.
I re-installed the IDE using the "latest release", so Oxygen RC yesterday, too.
I hope it will not contain some regressions about white space handling or save actions.

@amitjoy
Copy link
Contributor Author

amitjoy commented Jun 29, 2017

@maggu2810 I have amended the PR according your suggestions.

this.jobs = jobs;
this.thingUID = jobs.get(0).getThingUID();

boolean notMatched = jobs.stream().noneMatch(j -> j.getThingUID().equals(thingUID));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Miss I something?

Don't we need to ensure that allMatch == true.
None match will only be true if no one matches.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. definitely. It should better off with anyMatch -> if any of the elements doesn't have associated thing UID that matches the expected thing UID, it must throw IAE.


@Override
public void run() {
jobs.forEach(Job::run);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this differs from my suggested implementation a little bit (https://github.com/amitjoy/smarthome/compare/fix_range_events_delay...maggu2810:pr3752?expand=1#diff-2af4ee8503592a50a47a62e370e62fffR40).

forEach:
"Performs the given action for each element of the Iterable until all elements have been processed or the action throws an exception."

So, your one stopped if a job's run method throws a runtime excpetion (silently).
This could be hard to detect.

Mine logs a warning, and continue with the next job.

Do you want to use lambdas and stream whenever possible or why have modified it this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iterating over the list using the Iterable#forEach is inherently thread-safe while iterating through a collection. I opted Stream#forEach out of this because it will not give us ordered execution. This is my primary motive. I could add logging behavior to catch the Runtime Exception as well.

@amitjoy
Copy link
Contributor Author

amitjoy commented Jun 30, 2017

@maggu2810 Could you kindly have a look? I have amended this PR with your suggested improvements.

@maggu2810
Copy link
Contributor

[ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:1.0.0:compile (default-compile) on project org.eclipse.smarthome.binding.astro: Compilation failure: Compilation failure:
[ERROR] /home/travis/build/eclipse/smarthome/extensions/binding/org.eclipse.smarthome.binding.astro/src/main/java/org/eclipse/smarthome/binding/astro/internal/job/CompositeJob.java:[30]
[ERROR] public CompositeJob(List<Job> jobs) {
[ERROR] ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[ERROR] Implicit super constructor AbstractJob() is undefined. Must explicitly invoke another constructor
[ERROR] 1 problem (1 error)
[ERROR] -> [Help 1]

@amitjoy
Copy link
Contributor Author

amitjoy commented Jun 30, 2017

Ah. The other PR from @SJKA got merged. I will rebase this PR.

Copy link
Contributor

@maggu2810 maggu2810 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. The other PR from @SJKA got merged. I will rebase this PR.

IMHO there is no reason to hold the thing UID twice, now.

*/
public final class CompositeJob extends AbstractJob {

private final String thingUID;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop that member, it is part of AbstractJob already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Thanks for noticing.

checkArgument(jobs != null, "Jobs must not be null");
checkArgument(!jobs.isEmpty(), "Jobs must not be empty");

this.thingUID = thingUID;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop that line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to remove due to quick refactoring.

}

@Override
public String getThingUID() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop that member, it is part of AbstractJob already.

Also-by: Markus Rathgeb <maggu2810@gmail.com>
Signed-off-by: Amit Kumar Mondal <admin@amitinside.com>
@sjsf sjsf merged commit 0e2ad7f into eclipse-archived:master Jul 4, 2017
@sjsf
Copy link
Contributor

sjsf commented Jul 4, 2017

Thanks!

@amitjoy amitjoy deleted the fix_range_events_delay branch July 4, 2017 15:57
@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017
@kaikreuzer kaikreuzer added the bug label Dec 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants