Added Composite Jobs for range events if scheduled at same instant #3752
Conversation
There was a problem hiding this 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()); | |||
|
|||
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
6c03081
to
8670894
Compare
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. |
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? |
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? |
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(); | |||
} | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here two please.
005dae2
to
932bc5b
Compare
@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. |
There was a problem hiding this 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...
932bc5b
to
5bc2741
Compare
Wait a moment before merging, I will have a look at, too. |
@amitjoy Only a quick and dirty change for discussion: https://github.com/amitjoy/smarthome/compare/fix_range_events_delay...maggu2810:pr3752?expand=1 |
@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? |
@amitjoy Merge / cherry-pick that changes into your branch or integrate it manually. |
3707e90
to
9a9a396
Compare
@maggu2810 I have updated the PR according to suggestions. |
9a9a396
to
7cd0608
Compare
There was a problem hiding this 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; |
There was a problem hiding this comment.
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).
7cd0608
to
a156f01
Compare
@SJKA I have amended the PR. |
@@ -0,0 +1,47 @@ | |||
/** | |||
* Copyright (c) 2017 by the respective copyright holders. |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); | |||
} | |||
} | |||
|
|||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
a156f01
to
ed30e48
Compare
ed30e48
to
2bbe459
Compare
@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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
2bbe459
to
ec67aa5
Compare
|
||
@Override | ||
public void run() { | ||
jobs.forEach(Job::run); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
ec67aa5
to
59edea7
Compare
@maggu2810 Could you kindly have a look? I have amended this PR with your suggested improvements. |
|
Ah. The other PR from @SJKA got merged. I will rebase this PR. |
59edea7
to
31bd7f2
Compare
There was a problem hiding this 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop that line
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
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.
31bd7f2
to
931057f
Compare
Also-by: Markus Rathgeb <maggu2810@gmail.com> Signed-off-by: Amit Kumar Mondal <admin@amitinside.com>
931057f
to
9db1b0b
Compare
Thanks! |
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.