Repository navigation
[fix][ml] Fix active cursor position tracking after untracking - #26629
Merged
Merged
Conversation
lhotari
reviewed
Sep 17, 2026
lhotari
left a comment
Member
There was a problem hiding this comment.
Thanks for catching this and fixing the cursor untracking state and covering both flush paths and restoration. The fix looks sound; I have one suggestion to keep the container regression tests lightweight.
lhotari
approved these changes
Sep 17, 2026
lhotari
left a comment
Member
There was a problem hiding this comment.
LGTM. The 14 lightweight container cases and the separate seek regression pass locally. Please also refresh the PR’s verification description and test command to reflect this split; it still says all new tests start real services.
lhotari
pushed a commit
that referenced
this pull request
Sep 23, 2026
(cherry picked from commit 467159e)
This file contains hidden or 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
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.
Motivation
When
ActiveManagedCursorContainerImplflushes anupdateCursor(cursor, null)oradd(cursor, null)operation, it removes the cursor from position tracking but retains its old position. Subsequent queries can return stale ranks, and restoring a position can leave the cursor detached or incorrectly linked. Repeated null updates can also attempt to unlink an already-untracked node and corrupt the remaining list.This fixes a pre-existing container correctness issue identified while reviewing #26511. Normal broker deactivation uses
removeCursor(), so these tests do not establish a failure in that production path.Modifications
add()and real cursor seek callbacks.Verifying this change
The new tests start real ZooKeeper and BookKeeper services and create managed ledgers and cursors through
ManagedLedgerFactoryImpl, without mocking cursors or containers. Null untracking is invoked through the container API; seek-based restoration exercisesManagedCursorImpl→ManagedLedgerImpl→ the active cursor container.All 14 new cases fail on the unpatched implementation with incorrect rank or slowest-position results. After the fix, all 73 cases across the three container test classes pass.
quickCheckalso passes locally.Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes