Skip to content

Issue 2422: Fix SyncLedgerIterator.hasNext() failing to iterate across ZK ledger ranges#4731

Merged
wenbingshen merged 1 commit into
apache:masterfrom
geniusjoe:bugfix/SyncLedgerIterator-hasNext
Mar 24, 2026
Merged

Issue 2422: Fix SyncLedgerIterator.hasNext() failing to iterate across ZK ledger ranges#4731
wenbingshen merged 1 commit into
apache:masterfrom
geniusjoe:bugfix/SyncLedgerIterator-hasNext

Conversation

@geniusjoe

@geniusjoe geniusjoe commented Mar 17, 2026

Copy link
Copy Markdown
Contributor

Fix #2422

Related PR: #2457

Motivation

The SyncLedgerIterator.hasNext() method in BookKeeper.java has a logic bug that causes it to stop iterating prematurely when ledgers span across multiple ZooKeeper ranges.

ZooKeeper stores ledgers in a hierarchical tree structure (e.g. /ledgers/00/0000/...). Each range (znode) holds at most 10,000 leaf-level ledger IDs.
The 10,000 limit is determined by the 4-digit last level (0000-9999) as defined in LegacyHierarchicalLedgerManager (2-4-4 split) and LongHierarchicalLedgerManager (3-4-4-4-4 split). When the number of ledgers exceeds 10,000, they are distributed across multiple ranges.
image

The original hasNext() implementation had the following logic:

@Override
public boolean hasNext() throws IOException {
    parent.checkClosed();
    if (currentRange != null) {
        if (currentRange.hasNext()) {
            return true;
        }
    } else if (iterator.hasNext()) {
        return true;
    }
    return false;
}

When currentRange is non-null but exhausted (no more elements), it falls through to return false without checking iterator.hasNext() for more ranges.

Changes

  1. Fix SyncLedgerIterator.hasNext() in BookKeeper.java:
    Simplified the conditional logic to properly fall through to iterator.hasNext() when the current range is exhausted
  2. Add testListLedgersLargeScale() test in LedgerMetadataTest.java
    Added a new test case with numOfLedgers = 10010 (> 10,000 per-range limit) to verify cross-range iteration works correctly.

@hanmz hanmz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@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, good catch

@wenbingshen wenbingshen 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

@wenbingshen wenbingshen merged commit 8a8f4d6 into apache:master Mar 24, 2026
34 of 36 checks passed
@hangc0276 hangc0276 added this to the 4.18.0 milestone May 27, 2026
StevenLuMT pushed a commit that referenced this pull request Jun 15, 2026
liudezhi2098 pushed a commit to ascentstream/bookkeeper that referenced this pull request Jun 16, 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.

BP-42: New Client API - list ledgers

6 participants