Skip to content

[fix][ml] Fix active cursor position tracking after untracking - #26629

Merged
lhotari merged 3 commits into
apache:masterfrom
Denovo1998:fix-active-cursor-untracking
Sep 17, 2026
Merged

lhotari merged 3 commits into
apache:masterfrom
Denovo1998:fix-active-cursor-untracking

Conversation

@Denovo1998

Copy link
Copy Markdown
Contributor

Motivation

When ActiveManagedCursorContainerImpl flushes an updateCursor(cursor, null) or add(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

  • Clear the stored position when incremental processing removes a node from position tracking.
  • Clear the position and both list links when a rebuild excludes an untracked cursor, allowing subsequent incremental insertion.
  • Only schedule removal for nodes with a tracked position, making null updates safe for already-untracked and never-tracked nodes.
  • Add regression tests covering incremental and rebuild paths, repeated untracking, and restoration through both add() and real cursor seek callbacks.

Verifying this change

  • Make sure that the change passes the CI checks.

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 exercises ManagedCursorImpl → 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. quickCheck also passes locally.

./gradlew :managed-ledger:test \
  --tests ActiveManagedCursorContainerUntrackingTest \
  --tests ActiveManagedCursorContainerTest \
  --tests ActiveManagedCursorContainerRetentionTest \
  -PtestRetryCount=0 -PtestFailFast=false

./gradlew quickCheck

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

@lhotari lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@Denovo1998
Denovo1998 requested a review from lhotari September 17, 2026 14:38

@lhotari lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 lhotari added this to the 5.0.0 milestone Sep 17, 2026
@lhotari
lhotari merged commit 467159e into apache:master Sep 17, 2026
43 checks passed
@Denovo1998
Denovo1998 deleted the fix-active-cursor-untracking branch September 18, 2026 00:04
dao-jun pushed a commit to ascentstream/pulsar that referenced this pull request Sep 20, 2026
dao-jun pushed a commit to ascentstream/pulsar that referenced this pull request Sep 20, 2026
dao-jun pushed a commit to ascentstream/pulsar that referenced this pull request Sep 20, 2026
lhotari pushed a commit that referenced this pull request Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants