-
Notifications
You must be signed in to change notification settings - Fork 9
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
Fix calculation of timeout before next change run #180
Conversation
1. Changes is not a list, but a mapping from name to list of `Change` objects. 2. Change with a long timeout should not postpone run of scheduled checks. 3. A newly added change has to be executed without delay caused by other changes.
@@ -192,5 +203,6 @@ def _add_change_to_queue(self, change): | |||
def stop(self, change: Change): | |||
_LOG.info('Stopping controller with additional change: {}'.format(change.get_name() if change else None)) | |||
# clear all pending 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.
Was this a TODO
comment? I don't see that we clear anything here.
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 think it should be removed
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'm fine to merge as it is right now
time_till_next_run = change.time_till_next_run() | ||
if min_time_till_next_change_run is None: | ||
min_time_till_next_change_run = time_till_next_run | ||
else: |
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.
else: | |
elif time_till_next_run is not None: |
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.
but it's nitpicking feel free to merge as it is.
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.
What is semantics of None
? The method is not even supposed to return that (not optional).
@@ -192,5 +203,6 @@ def _add_change_to_queue(self, change): | |||
def stop(self, change: Change): |
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.
def stop(self, change: Change): | |
def stop(self, change: Optional[Change]): |
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.
nitpicking again
@@ -192,5 +203,6 @@ def _add_change_to_queue(self, change): | |||
def stop(self, change: Change): | |||
_LOG.info('Stopping controller with additional change: {}'.format(change.get_name() if change else None)) | |||
# clear all pending 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.
I think it should be removed
👍 |
1 similar comment
👍 |
Change
objects.Internal ticket: 789
Review