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

Notification#BeginExecuteNotification(): SetNextNotification() correctly #6896

Merged
merged 1 commit into from Feb 22, 2019

Conversation

@Al2Klimov
Copy link
Contributor

commented Jan 11, 2019

fixes #5561

@Al2Klimov Al2Klimov requested a review from dnsmichi Jan 11, 2019

@dnsmichi

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

Please explain the change and add test output from before and after the patch.

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

Will be done after the user affected by the problem has tested it.

@dnsmichi

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

I'm more interested in the change from now towards GetLastHardStateChange() - please explain why this solves this in your opinion.

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2019

I just copied the formula from the enclosing if.

@dnsmichi

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

There's a difference with now vs GetLastHardStateChange where it is unclear to me which consequences this has.

@dnsmichi

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

if (timesBegin != Empty && timesBegin >= 0 && now < checkable->GetLastHardStateChange() + timesBegin) {

already checks the boundaries here, which is why we can just set next_notification to the delayed value.

@mdetrano can you test whether this fixes your reported problem?

@mdetrano

This comment has been minimized.

Copy link

commented Feb 21, 2019

Hello,

Finally had a chance to build and test with this fix merged in. Looks good. Notifications go out at the correct times when the begin time is not "0". No longer matters if the "next notification" falls after or before the calculated "begin" time. Should be sufficient to fix Issue #5561.

@dnsmichi

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

Cool, thanks a lot for testing and also for keeping up with us busy developers :)

@dnsmichi dnsmichi added this to the 2.11.0 milestone Feb 22, 2019

@dnsmichi

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

Thank you @Al2Klimov for taking care here 👍

@dnsmichi dnsmichi merged commit 30d98b4 into master Feb 22, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@dnsmichi dnsmichi deleted the bugfix/notification-delay-5561 branch Feb 22, 2019

@dnsmichi dnsmichi modified the milestones: 2.11.0, 2.10.4 Mar 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.