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

MINOR: Ensure LocalLog.flush() is immune to recoveryPoint change by different thread #11814

Conversation

kowshik
Copy link
Contributor

@kowshik kowshik commented Feb 26, 2022

Issue:
Imagine a scenario where two threads T1 and T2 are inside UnifiedLog.flush() concurrently:

  • KafkaScheduler thread T1 -> The periodic work calls LogManager.flushDirtyLogs() which in turn calls UnifiedLog.flush(). For example, this can happen due to log.flush.scheduler.interval.ms here.
  • KafkaScheduler thread T2 -> A UnifiedLog.flush() call is triggered asynchronously during segment roll here.

Supposing if thread T1 advances the recovery point beyond the flush offset of thread T2, then this could trip the check within LogSegments.values() here for thread T2, when it is called from LocalLog.flush() here. The exception causes the KafkaScheduler thread to die, which is not desirable.

Fix:
We fix this by ensuring that LocalLog.flush() is immune to the case where the recoveryPoint advances beyond the flush offset.

Tests:
I was able to test this manually by introducing barriers in the code to help simulate the race condition. As such, this is a hard case to write an automated unit test for, so I haven't added a new test case in this PR. So I'm mostly just relying on code review and also ensure there are no regressions in existing tests.

@kowshik
Copy link
Contributor Author

kowshik commented Feb 26, 2022

cc @junrao @lbradstreet for review

@kowshik kowshik changed the title MINOR: Ensure LocalLog.flush is immune to recoveryPoint change by different thread MINOR: Ensure LocalLog.flush() is immune to recoveryPoint change by different thread Feb 26, 2022
Copy link
Contributor

@junrao junrao left a comment

Choose a reason for hiding this comment

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

@kowshik : Thanks for the PR. LGTM. Are the test failures related?

This issue could occur even with flushing on rolled segments. In general, we could have a pool of background threads for flushing rolled log segments. It's possible for the same partition's log to be rolled quickly and flushed by different threads in parallel.

@kowshik
Copy link
Contributor Author

kowshik commented Mar 1, 2022

@junrao Thanks for the review! I checked the test failures, and they look unrelated to this PR. I agree, your suggestion is a good way to simplify the code and it will be a lot more maintainable too. I have opened KAFKA-13701 to track the improvement.

@junrao junrao merged commit 67e99a4 into apache:trunk Mar 1, 2022
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.

2 participants