Skip to content

[fix][ml] Fix backlog estimation during ledger rollover with pending writes - #26647

Merged
nodece merged 4 commits into
apache:masterfrom
Denovo1998:fix-cursor-backlog-estimate-during-rollover
Sep 20, 2026
Merged

nodece merged 4 commits into
apache:masterfrom
Denovo1998:fix-cursor-backlog-estimate-during-rollover

Conversation

@Denovo1998

Copy link
Copy Markdown
Contributor

Follow-up to #26184.

Motivation

After ledger rollover, a caught-up cursor can move to currentLedger:-1 while the first write to the new ledger is still pending. Since currentLedgerEntries includes pending writes, the check introduced in #26184 can reject this valid state and cause getEstimatedSizeSinceMarkDeletePosition() to throw, potentially skipping a compaction check.

The same check also incorrectly accepts nonnegative mark-delete entry IDs on an empty current ledger.

Modifications

  • Recognize the exact currentLedger:-1 sentinel without relying on currentLedgerEntries.
  • Snapshot the current ledger handle before checking its ID.
  • Add two regression tests to ManagedLedgerBkTest, using real BookKeeper operations without mocks or direct internal-state mutation. Cover a pending first write during rollover and an invalid nonnegative mark-delete entry ID on an empty ledger.
  • Verify that backlog estimation and reading work after the pending write completes.

Verifying this change

  • Make sure that the change passes the CI checks.

Both new regression tests fail on the original implementation for the expected reasons. After the fix, all 9 selected tests and quickCheck pass locally:

./gradlew :managed-ledger:test \
  --tests 'ManagedLedgerBkTest.testEstimatedUnackedSize*' \
  --tests 'ManagedCursorTest.testEstimated*' \
  -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.

LGTM. Thanks for fixing the rollover case and adding regression coverage for both pending writes and invalid positions.

@nodece
nodece merged commit 776248a into apache:master Sep 20, 2026
43 checks passed
@Denovo1998
Denovo1998 deleted the fix-cursor-backlog-estimate-during-rollover branch September 20, 2026 02:51
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.

4 participants