Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,18 @@ private long getLedgerToRereplicateFromHierarchy(String parent, long depth)
return -1;
}

Collections.shuffle(children);
if (depth == 3) {
// 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
// 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());
}
Copy link
Member

Choose a reason for hiding this comment

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

This code isn't very readable; I recommend optimizing it.
What does the magic number 3 mean? I recommend including constants to explain their names.


while (children.size() > 0) {
String tryChild = children.get(0);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -171,6 +173,54 @@ public Long call() {
});
}

/**
* 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
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<Long> 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<Long> processedLedgers = new ArrayList<>();
for (int i = 0; i < testLedgers.size(); i++) {
Future<Long> future = getLedgerToReplicate(manager);
Long ledgerId = future.get(5, TimeUnit.SECONDS);
processedLedgers.add(ledgerId);
}

List<Long> 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.
Expand Down Expand Up @@ -675,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");
Expand Down
Loading