Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jul 4, 2017

Conversation

Projects
None yet
4 participants
@amitjoy
Copy link
Member

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.

@sjka
Copy link
Contributor

left a comment

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;

This comment has been minimized.

Copy link
@sjka

sjka Jun 27, 2017

Contributor

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.

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 27, 2017

Author Member

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());

This comment has been minimized.

Copy link
@sjka

sjka Jun 27, 2017

Contributor

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...)

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 27, 2017

Author Member

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.

This comment has been minimized.

Copy link
@sjka

sjka Jun 27, 2017

Contributor

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

@sjka sjka added the Binding-Astro label Jun 27, 2017

@amitjoy amitjoy force-pushed the amitjoy:fix_range_events_delay branch from 6c03081 to 8670894 Jun 27, 2017

@amitjoy

This comment has been minimized.

Copy link
Member Author

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.

@sjka

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member Author

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?

@sjka

This comment has been minimized.

Copy link
Contributor

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();
}

This comment has been minimized.

Copy link
@sjka

sjka Jun 27, 2017

Contributor

Here two please.

@amitjoy amitjoy force-pushed the amitjoy:fix_range_events_delay branch 5 times, most recently from 005dae2 to 932bc5b Jun 27, 2017

@amitjoy

This comment has been minimized.

Copy link
Member Author

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.

@sjka

sjka approved these changes Jun 27, 2017

Copy link
Contributor

left a comment

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

@amitjoy amitjoy force-pushed the amitjoy:fix_range_events_delay branch from 932bc5b to 5bc2741 Jun 27, 2017

@maggu2810

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2017

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

@maggu2810

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2017

@amitjoy

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2017

@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 amitjoy:fix_range_events_delay branch 2 times, most recently from 3707e90 to 9a9a396 Jun 29, 2017

@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

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2017

@maggu2810 I have updated the PR according to suggestions.

@amitjoy amitjoy force-pushed the amitjoy:fix_range_events_delay branch from 9a9a396 to 7cd0608 Jun 29, 2017

@sjka
Copy link
Contributor

left a comment

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;

This comment has been minimized.

Copy link
@sjka

sjka Jun 29, 2017

Contributor

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 amitjoy force-pushed the amitjoy:fix_range_events_delay branch from 7cd0608 to a156f01 Jun 29, 2017

@amitjoy

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2017

@sjka I have amended the PR.

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

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jun 29, 2017

Contributor

see comment below

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

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

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jun 29, 2017

Contributor

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

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 29, 2017

Author Member

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.

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jun 29, 2017

Contributor

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?

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 29, 2017

Author Member

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

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jun 29, 2017

Contributor

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

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 29, 2017

Author Member

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);
}
}

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jun 29, 2017

Contributor

This trailing whitespaces indicates that your formatter is not working.

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 29, 2017

Author Member

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.

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jun 29, 2017

Contributor

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 amitjoy force-pushed the amitjoy:fix_range_events_delay branch from a156f01 to ed30e48 Jun 29, 2017

@amitjoy amitjoy force-pushed the amitjoy:fix_range_events_delay branch from ed30e48 to 2bbe459 Jun 29, 2017

@amitjoy

This comment has been minimized.

Copy link
Member Author

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));

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jun 29, 2017

Contributor

Miss I something?

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

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 29, 2017

Author Member

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.

@amitjoy amitjoy force-pushed the amitjoy:fix_range_events_delay branch from 2bbe459 to ec67aa5 Jun 29, 2017


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

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jun 29, 2017

Contributor

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?

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 29, 2017

Author Member

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 amitjoy force-pushed the amitjoy:fix_range_events_delay branch from ec67aa5 to 59edea7 Jun 29, 2017

@amitjoy

This comment has been minimized.

Copy link
Member Author

commented Jun 30, 2017

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

@maggu2810

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2017

[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

This comment has been minimized.

Copy link
Member Author

commented Jun 30, 2017

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

@amitjoy amitjoy force-pushed the amitjoy:fix_range_events_delay branch from 59edea7 to 31bd7f2 Jun 30, 2017

@maggu2810
Copy link
Contributor

left a comment

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;

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jun 30, 2017

Contributor

Drop that member, it is part of AbstractJob already.

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 30, 2017

Author Member

Good catch. Thanks for noticing.

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

this.thingUID = thingUID;

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jun 30, 2017

Contributor

Drop that line

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 30, 2017

Author Member

Forgot to remove due to quick refactoring.

}

@Override
public String getThingUID() {

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jun 30, 2017

Contributor

Drop that member, it is part of AbstractJob already.

@amitjoy amitjoy force-pushed the amitjoy:fix_range_events_delay branch from 31bd7f2 to 931057f Jun 30, 2017

Added composite job for range events if scheduled at same time instant
Also-by: Markus Rathgeb <maggu2810@gmail.com>
Signed-off-by: Amit Kumar Mondal <admin@amitinside.com>

@amitjoy amitjoy force-pushed the amitjoy:fix_range_events_delay branch from 931057f to 9db1b0b Jun 30, 2017

@sjka sjka merged commit 0e2ad7f into eclipse:master Jul 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sjka

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2017

Thanks!

@amitjoy amitjoy deleted the amitjoy:fix_range_events_delay branch Jul 4, 2017

@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 join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.