fix: unregistering topic translates to multiple redundant Raft proposals #979
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #979 +/- ##
==========================================
+ Coverage 80.21% 80.25% +0.04%
==========================================
Files 179 179
Lines 21550 21548 -2
==========================================
+ Hits 17286 17294 +8
Misses 3479 3479
+ Partials 785 775 -10 ☔ View full report in Codecov by Sentry. |
hungryjang
reviewed
Feb 1, 2025
hungryjang
approved these changes
Feb 3, 2025
During topic unregistration, an early return after each log stream unregistration prevented cacheCompleteCB from being called. Without this callback, proposeWithGuarantee interpreted the operation as incomplete and retried the entire topic unregistration multiple times. This resulted in linear scaling of unregistration time with the number of log streams (approximately 100ms per log stream) and additional consensus overhead. Remove the premature return to ensure cacheCompleteCB is called after all log streams are processed, allowing the entire operation to be handled in a single Raft proposal.
ijsong
force-pushed
the
reduce_redundant_raft_proposals_in_topic_unregister
branch
from
1a2a050 to
16093f0
Compare
ijsong
approved these changes
Feb 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does
The current implementation of topic unregistration relies on proposeWithGuarantee's retry mechanism when deleting topics with multiple log streams. When a topic containing multiple log streams is unregistered, the control flow returns prematurely after deleting each log stream. This premature return prevents cacheCompleteCB from being called, which proposeWithGuarantee interprets as an incomplete operation that needs to be retried causing the unregistration time to scale linearly with the number of log streams (approximately 100ms per stream) and generates unnecessary consensus overhead.
The fix consolidates all log stream unregistrations into a single Raft proposal by removing the premature return.
Anything else
With topics containing over 1 billion log streams, state machine operations could still take longer than 100ms, potentially triggering the same retry behavior addressed in this fix. However, this would not affect correctness as the retry mechanism would continue to function as intended. At such scale, other system constraints would likely manifest before this particular issue would resurface.