[FIX] fix SyncLedgerIterator.hasNext() failing to iterate across ZK ledger ranges (#4731)

Author: geniusjoe

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java

@@ -1583,17 +1583,20 @@ public SyncLedgerIterator(LedgerRangeIterator iterator, ListLedgersResultImpl pa
             this.parent = parent;
         }
 
-        @Override
+        /**
+         * ZooKeeper stores ledgers in a hierarchical tree (e.g. /ledgers/00/0000/...).
+         * {@code iterator} (LedgerRangeIterator) traverses the intermediate tree nodes (ranges/buckets),
+         * while {@code currentRange} iterates over the leaf-level ledger IDs within a single range.
+         *
+         * Therefore, when {@code currentRange} has no more leaf nodes, we need to check
+         * {@code iterator.hasNext()} to determine if there are more ranges to advance to.
+         */
         public boolean hasNext() throws IOException {
             parent.checkClosed();
-            if (currentRange != null) {
-                if (currentRange.hasNext()) {
-                    return true;
-                }
-            } else if (iterator.hasNext()) {
+            if (currentRange != null && currentRange.hasNext()) {
                 return true;
             }
-            return false;
+            return iterator.hasNext();
         }
 
         @Override

bookkeeper-server/src/test/java/org/apache/bookkeeper/client/api/LedgerMetadataTest.java

@@ -190,4 +190,150 @@ public void testListLedgers()
         }
     }
 
+    /**
+     * Unlike {@link #testListLedgers()} which only uses 10 ledgers within a single ZK range,
+     * this test creates 10010 ledgers to verify cross-range iteration.
+     *
+     * <p>The HierarchicalLedgerManager stores ledgers in a ZK tree where each range (znode)
+     * holds at most 10,000 leaf nodes. This limit is determined by the 4-digit last level of
+     * the ledger ID path (0000-9999), as defined in:
+     * <ul>
+     *   <li>{@code LegacyHierarchicalLedgerManager} — 2-4-4 split,
+     *       path: {root}/{level1}/{level2}/L{level3}</li>
+     *   <li>{@code LongHierarchicalLedgerManager} — 3-4-4-4-4 split,
+     *       path: {root}/{level0}/{level1}/{level2}/{level3}/L{level4}</li>
+     * </ul>
+     *
+     * <p>By using numOfLedgers = 10010 (> 10000), ledgers will span across multiple ranges,
+     * exercising the SyncLedgerIterator's ability to advance from one exhausted range to the
+     * next via LedgerRangeIterator.
+     */
+    @Test
+    public void testListLedgersLargeScale()
+            throws Exception {
+        // 10010 > 10000 (max ledgers per ZK range, determined by 4-digit last level 0000-9999),
+        // ensuring cross-range iteration is covered
+        int numOfLedgers = 10010;
+
+        ClientConfiguration conf = new ClientConfiguration();
+        conf.setMetadataServiceUri(zkUtil.getMetadataServiceUri());
+
+        try (BookKeeper bkc = BookKeeper.newBuilder(conf).build();) {
+            long[] ledgerIds = new long[numOfLedgers];
+            for (int i = 0; i < numOfLedgers; i++) {
+
+                try (WriteHandle l = bkc
+                        .newCreateLedgerOp()
+                        .withDigestType(DigestType.CRC32)
+                        .withPassword("testPasswd".getBytes())
+                        .execute()
+                        .get();) {
+                    ledgerIds[i] = l.getId();
+                }
+            }
+
+            try (ListLedgersResult result = FutureUtils.result(bkc.newListLedgersOp().execute());) {
+                int count = 0;
+
+                for (long ledgerId : result.toIterable()) {
+                    assertEquals(ledgerIds[count++], ledgerId);
+                }
+
+                assertEquals(numOfLedgers, count, "Unexpected ledgers count");
+                try {
+                    result.iterator();
+                    fail("Should thrown error");
+                } catch (IllegalStateException e) {
+                    // ok
+                }
+                try {
+                    result.toIterable();
+                    fail("Should thrown error");
+                } catch (IllegalStateException e) {
+                    // ok
+                }
+            }
+
+            try (ListLedgersResult result = FutureUtils.result(bkc.newListLedgersOp().execute());) {
+                int count = 0;
+
+                for (LedgersIterator iterator = result.iterator(); iterator.hasNext();) {
+                    long ledgerId = iterator.next();
+                    assertEquals(ledgerIds[count++], ledgerId);
+
+                }
+                assertEquals(numOfLedgers, count, "Unexpected ledgers count");
+                try {
+                    result.iterator();
+                    fail("Should thrown error");
+                } catch (IllegalStateException e) {
+                    // ok
+                }
+                try {
+                    result.toIterable();
+                    fail("Should thrown error");
+                } catch (IllegalStateException e) {
+                    // ok
+                }
+            }
+        }
+
+        // check closed
+        {
+            ListLedgersResult result = FutureUtils.result(bkc.newListLedgersOp().execute());
+            result.close();
+            try {
+                result.toIterable();
+                fail("Should thrown error");
+            } catch (IllegalStateException e) {
+                // ok
+            }
+
+            try {
+                result.iterator();
+                fail("Should thrown error");
+            } catch (IllegalStateException e) {
+                // ok
+            }
+        }
+
+        { // iterator
+            ListLedgersResult result = FutureUtils.result(bkc.newListLedgersOp().execute());
+            LedgersIterator it = result.iterator();
+            result.close();
+            try {
+                it.hasNext();
+                fail("Should thrown error");
+            } catch (IllegalStateException e) {
+                // ok
+            }
+
+            try {
+                it.next();
+                fail("Should thrown error");
+            } catch (IllegalStateException e) {
+                // ok
+            }
+        }
+
+        { // iterable
+            ListLedgersResult result = FutureUtils.result(bkc.newListLedgersOp().execute());
+            Iterator<Long> it = result.toIterable().iterator();
+            result.close();
+            try {
+                it.hasNext();
+                fail("Should thrown error");
+            } catch (IllegalStateException e) {
+                // ok
+            }
+
+            try {
+                it.next();
+                fail("Should thrown error");
+            } catch (IllegalStateException e) {
+                // ok
+            }
+        }
+    }
+
 }