From 004a636dd44f532673485c8aebb4bf664d43b4d7 Mon Sep 17 00:00:00 2001 From: gaozhangmin Date: Mon, 28 Jul 2025 14:11:36 +0800 Subject: [PATCH 1/5] let the worker can rerplicate the fresh ledgers at first. --- .../meta/ZkLedgerUnderreplicationManager.java | 7 ++- .../TestLedgerUnderreplicationManager.java | 51 +++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerUnderreplicationManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerUnderreplicationManager.java index 4118e191319..13a7baa6d1f 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerUnderreplicationManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerUnderreplicationManager.java @@ -567,7 +567,12 @@ private long getLedgerToRereplicateFromHierarchy(String parent, long depth) return -1; } - Collections.shuffle(children); + if (depth == 3) { + // Avoid workers want to get the same ledger lock. + Collections.shuffle(children); + } else { + Collections.sort(children, Collections.reverseOrder()); + } while (children.size() > 0) { String tryChild = children.get(0); diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestLedgerUnderreplicationManager.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestLedgerUnderreplicationManager.java index 3e418226e61..060873a255b 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestLedgerUnderreplicationManager.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestLedgerUnderreplicationManager.java @@ -30,6 +30,7 @@ import java.nio.charset.Charset; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.HashSet; import java.util.Iterator; import java.util.List; @@ -43,6 +44,7 @@ import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Collectors; import lombok.Cleanup; import org.apache.bookkeeper.conf.ServerConfiguration; import org.apache.bookkeeper.conf.TestBKConfiguration; @@ -171,6 +173,55 @@ public Long call() { }); } + /** + * Test that larger ledgerIds are processed first when getting ledgers to rereplicate. + * This verifies the sorting behavior in getLedgerToRereplicateFromHierarchy. + * Specifically tests the Collections.sort(children, Collections.reverseOrder()) at line 564. + */ + @Test + public void testLargerLedgerIdProcessedFirst() throws Exception { + // 4294950639 (0xffffbeefL): /ledgers/0000/0000/ffff/beef/urL4294950639 + // 4207853295 (0xdeadbeefL): /ledgers/0000/0000/dead/beef/urL4207853295 + // 3735928559 (0xbeefcafeL): /ledgers/0000/0000/beef/cafe/urL3735928559 + // 3203386110 (0xfacebeefL): /ledgers/0000/0000/face/beef/urL3203386110 + Set testLedgers = new HashSet<>(); + testLedgers.add(0xffffbeefL); + testLedgers.add(0xdeadbeefL); + testLedgers.add(0xbeefcafeL); + testLedgers.add(0xfacebeefL); + + String missingReplica = "localhost:3181"; + + @Cleanup + LedgerUnderreplicationManager manager = lmf1.newLedgerUnderreplicationManager(); + + for (Long ledgerId : testLedgers) { + manager.markLedgerUnderreplicated(ledgerId, missingReplica); + } + + List processedLedgers = new ArrayList<>(); + for (int i = 0; i < testLedgers.size(); i++) { + Future future = getLedgerToReplicate(manager); + Long ledgerId = future.get(5, TimeUnit.SECONDS); + processedLedgers.add(ledgerId); + } + + List expectedLedgers = new ArrayList<>(testLedgers) + .stream().sorted(Collections.reverseOrder()).collect(Collectors.toList()); + + assertEquals("Should have processed all test ledgers",expectedLedgers, processedLedgers); + + for (Long ledgerId : processedLedgers) { + assertTrue("Processed ledger should be in test set", testLedgers.contains(ledgerId)); + } + + Long firstProcessed = processedLedgers.get(0); + Long expectedFirst = 0xffffbeefL; + + assertEquals("Larger ledger ID should be processed first", expectedFirst, firstProcessed); + } + + /** * Test basic interactions with the ledger underreplication * manager. From af28e54da750a29ba3744ff2e199567f7a127d48 Mon Sep 17 00:00:00 2001 From: gaozhangmin Date: Thu, 31 Jul 2025 11:12:56 +0800 Subject: [PATCH 2/5] fix comments --- .../bookkeeper/meta/ZkLedgerUnderreplicationManager.java | 6 ++++++ .../replication/TestLedgerUnderreplicationManager.java | 3 +-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerUnderreplicationManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerUnderreplicationManager.java index 13a7baa6d1f..f3a5984cc3b 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerUnderreplicationManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerUnderreplicationManager.java @@ -571,6 +571,12 @@ private long getLedgerToRereplicateFromHierarchy(String parent, long depth) // Avoid workers want to get the same ledger lock. Collections.shuffle(children); } else { + // Sort children in descending order for depths 0-2 to prioritize larger ledger IDs + // The hierarchical path is built based on hexadecimal representation of ledger IDs + // Larger ledger IDs will appear first in hexadecimal string comparison + // By using reverse order sorting, we ensure that paths containing larger ledger IDs + // are visited first during directory traversal, implementing the business requirement + // of "larger ledger IDs should be processed first" Collections.sort(children, Collections.reverseOrder()); } diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestLedgerUnderreplicationManager.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestLedgerUnderreplicationManager.java index 060873a255b..2e215f72bb4 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestLedgerUnderreplicationManager.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestLedgerUnderreplicationManager.java @@ -209,8 +209,7 @@ public void testLargerLedgerIdProcessedFirst() throws Exception { List expectedLedgers = new ArrayList<>(testLedgers) .stream().sorted(Collections.reverseOrder()).collect(Collectors.toList()); - assertEquals("Should have processed all test ledgers",expectedLedgers, processedLedgers); - + assertEquals("Should have processed all test ledgers", expectedLedgers, processedLedgers); for (Long ledgerId : processedLedgers) { assertTrue("Processed ledger should be in test set", testLedgers.contains(ledgerId)); } From 57b50a72b239d6fc47ec372db6243ee3908f8b00 Mon Sep 17 00:00:00 2001 From: Zhangao Date: Fri, 8 Aug 2025 10:54:33 +0800 Subject: [PATCH 3/5] Update bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestLedgerUnderreplicationManager.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../replication/TestLedgerUnderreplicationManager.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestLedgerUnderreplicationManager.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestLedgerUnderreplicationManager.java index 2e215f72bb4..2a8276386ef 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestLedgerUnderreplicationManager.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestLedgerUnderreplicationManager.java @@ -174,8 +174,8 @@ public Long call() { } /** - * Test that larger ledgerIds are processed first when getting ledgers to rereplicate. - * This verifies the sorting behavior in getLedgerToRereplicateFromHierarchy. + * Test that larger ledgerIds are processed first when getting ledgers to replicate. + * This verifies the sorting behavior in getLedgerToReplicateFromHierarchy. * Specifically tests the Collections.sort(children, Collections.reverseOrder()) at line 564. */ @Test From 23694472098ac55fab1e43cd665bf05d4d03d28e Mon Sep 17 00:00:00 2001 From: Zhangao Date: Fri, 8 Aug 2025 10:54:41 +0800 Subject: [PATCH 4/5] Update bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerUnderreplicationManager.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../apache/bookkeeper/meta/ZkLedgerUnderreplicationManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerUnderreplicationManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerUnderreplicationManager.java index f3a5984cc3b..31a84c66bb0 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerUnderreplicationManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerUnderreplicationManager.java @@ -568,7 +568,7 @@ private long getLedgerToRereplicateFromHierarchy(String parent, long depth) } if (depth == 3) { - // Avoid workers want to get the same ledger lock. + // Avoid workers wanting to get the same ledger lock. Collections.shuffle(children); } else { // Sort children in descending order for depths 0-2 to prioritize larger ledger IDs From 73662bee1335d6b0be72f7922a881c47ef628618 Mon Sep 17 00:00:00 2001 From: gaozhangmin Date: Mon, 11 Aug 2025 15:12:12 +0800 Subject: [PATCH 5/5] fix texts --- .../replication/TestLedgerUnderreplicationManager.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestLedgerUnderreplicationManager.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestLedgerUnderreplicationManager.java index 2a8276386ef..d9f515365f2 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestLedgerUnderreplicationManager.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestLedgerUnderreplicationManager.java @@ -725,8 +725,8 @@ public void testHierarchyCleanup() throws Exception { final LedgerUnderreplicationManager replicaMgr = lmf1 .newLedgerUnderreplicationManager(); // 4 ledgers, 2 in the same hierarchy - long[] ledgers = { 0x00000000deadbeefL, 0x00000000deadbeeeL, - 0x00000000beefcafeL, 0x00000000cafed00dL }; + long[] ledgers = {0x00000000beefcafeL, 0x00000000cafed00dL, + 0x00000000deadbeeeL, 0x00000000deadbeefL}; for (long l : ledgers) { replicaMgr.markLedgerUnderreplicated(l, "localhost:3181");