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

Re-enable IdleStateMonitor #87

Merged
merged 1 commit into from
Dec 2, 2021
Merged

Re-enable IdleStateMonitor #87

merged 1 commit into from
Dec 2, 2021

Conversation

pihme
Copy link
Contributor

@pihme pihme commented Dec 1, 2021

This commit re-enables IdleStateMonitor and fixes it to make it usable.

Learnings:

  • we need to be mindful of multi-threading
  • the sequence of events is not always intuitive. E.g. when the stream processor is writing the follow-up events, then the listener calls for the new log entries and the skipped entries can come in before the stream processor finished writing all events.
  • Because of that, the old mechanism detected an idle state prematurely, i.e. after the first follow-up event was written to the log and immediately skipped by the record stream processor. It didn't wait whether more follow-up events are to come
  • Therefore, there is now a timer started when the first idle state is detected.
  • The timer delay may depend on the speed of the machine. So maybe we need to tune this going forward.

Related issues

closes #6
closes #71
closes #77

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

  • The changes are backwards compatibility with previous versions
  • If it fixes a bug then PRs are created to backport the fix

Testing:

  • There are unit/integration tests that verify all acceptance criterias of the issue
  • New tests are written to ensure backwards compatibility with further versions
  • The behavior is tested manually

Documentation:

  • Javadoc has been written
  • The documentation is updated

This commit re-enables IdleStateMonitor and fixes it to make it usable.

Learnings:
- we need to be mindful of multi-threading
- the sequence of events is not always intuitive. E.g. when the stream processor is writing the follow-up events, then the listener calls for the new log entries and the skipped entries can come in before the stream processor finished writing all events.
- Because of that, the old mechanism detected an idle state prematurely, i.e. after the first follow-up event was written to the log and immediately skipped by the record stream processor. It didn't wait whether more follow-up events are to come
- Therefore, there is now a timer started when the first idle state is detected.
- The timer delay may depend on the speed of the machine. So maybe we need to tune this going forward.
@pihme pihme requested a review from remcowesterhoud December 1, 2021 17:21
Copy link
Contributor

@remcowesterhoud remcowesterhoud left a comment

Choose a reason for hiding this comment

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

@pihme Great job getting this working 🏆

I have a few minor comments, please have a look at them and feel free to merge it after!

@pihme
Copy link
Contributor Author

pihme commented Dec 2, 2021

@remcowesterhoud Responded to your comments. Please have another look and close any conversations you feel are resolved, then let's discuss the remaining ones.

@pihme pihme merged commit 8c65930 into main Dec 2, 2021
@pihme pihme deleted the wait-for-idle-state branch December 2, 2021 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants