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

[OASIS-7827] fix: make _get_time() value the same throughout the loop #356

Merged
merged 3 commits into from
Aug 9, 2021

Conversation

Mat001
Copy link
Contributor

@Mat001 Mat001 commented Aug 4, 2021

Summary

  • Running event processor would sometimes produce Buffer error with negative timeout interval for retrieving an event from the queue. Reported by a Github user (customer). See BatchEventProcessor can try to deque event with negative timeout. #354
  • If the flushing_interval_deadline is between the first call of _get_time() and the second call of _get_time() then interval will be negative.

Test plan

Unit tests for the _run() function where the change is made already extensively cover the _run() and event processor functionality.
The change itself should prevent negative interval from occurring. To test that a larger load test would be required.

Issues

@Mat001 Mat001 requested a review from a team as a code owner August 4, 2021 16:30
@coveralls
Copy link

coveralls commented Aug 4, 2021

Coverage Status

Coverage increased (+0.003%) to 96.02% when pulling 4450f00 on mpirnovar/fix_negative_interval into 0cb19ce on master.

@@ -180,14 +180,15 @@ def _run(self):
"""
try:
while True:
if self._get_time() >= self.flushing_interval_deadline:
loop_time = self._get_time()
if loop_time >= self.flushing_interval_deadline:
self._flush_batch()
self.flushing_interval_deadline = self._get_time() + \
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change this ._get_time() to loop_time as well? - line 186

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was thinking about that yes. Thx for the pointer. Will implement and test.

Copy link
Contributor

@The-inside-man The-inside-man left a comment

Choose a reason for hiding this comment

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

All looks good to me. Noting that FSC will not pass till Config V2 is merged in. All other tests appear to pass which is good. Also noting to wait to merge till V2 Config is merged so we have a clean set of checks on this PR before merging.

@Mat001 Mat001 merged commit de91292 into master Aug 9, 2021
@Mat001 Mat001 deleted the mpirnovar/fix_negative_interval branch August 9, 2021 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants