Skip to content

Commit a5b6830

Browse files
author
xiangying
committed
Refactor addDefaultRackBookiesIfMinNumRacksIsEnforced to Remove Redundant Logic and Improve Robustness
### Motivation The current implementation of `addDefaultRackBookiesIfMinNumRacksIsEnforced` has the following issues: - **Redundant logic**: The initialization and merging process for collecting Bookie nodes on the default rack involves unnecessary data copying and checks. - **Potential robustness issue**: There is no null check for the address returned by `BookieNode.getAddr()`, which may result in `null` values being included in the exclusion set. - **Incorrect logging**: The log claims to print only the defaultRack Bookies that are being excluded, while it actually prints all the nodes, including those already in `excludeBookies`. ```java LOG.info("enforceMinNumRacksPerWriteQuorum is enabled, so Excluding bookies of defaultRack: {}", bookiesInDefaultRack); ``` --- ### Modification - When creating `bookiesInDefaultRack`, do not copy elements from `excludeBookies`. - Add a null check for the address returned by `BookieNode.getAddr()`. - Remove unnecessary null checks for `bookiesInDefaultRack`.
1 parent c9b893f commit a5b6830

File tree

1 file changed

+3
-3
lines changed

1 file changed

+3
-3
lines changed

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -364,16 +364,16 @@ protected Set<BookieId> addDefaultRackBookiesIfMinNumRacksIsEnforced(
364364
Set<BookieId> bookiesInDefaultRack = null;
365365
Set<Node> defaultRackLeaves = topology.getLeaves(getDefaultRack());
366366
for (Node node : defaultRackLeaves) {
367-
if (node instanceof BookieNode) {
367+
if (node instanceof BookieNode && ((BookieNode) node).getAddr() != null) {
368368
if (bookiesInDefaultRack == null) {
369-
bookiesInDefaultRack = new HashSet<BookieId>(excludeBookies);
369+
bookiesInDefaultRack = new HashSet<>();
370370
}
371371
bookiesInDefaultRack.add(((BookieNode) node).getAddr());
372372
} else {
373373
LOG.error("found non-BookieNode: {} as leaf of defaultrack: {}", node, getDefaultRack());
374374
}
375375
}
376-
if ((bookiesInDefaultRack == null) || bookiesInDefaultRack.isEmpty()) {
376+
if (bookiesInDefaultRack == null) {
377377
comprehensiveExclusionBookiesSet = excludeBookies;
378378
} else {
379379
comprehensiveExclusionBookiesSet = new HashSet<BookieId>(excludeBookies);

0 commit comments

Comments
 (0)