Copilot commented on code in PR #4652:
URL: https://github.com/apache/bookkeeper/pull/4652#discussion_r2435567618
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java:
##########
@@ -359,32 +359,31 @@ public void uninitalize() {
*/
protected Set<BookieId> addDefaultRackBookiesIfMinNumRacksIsEnforced(
Set<BookieId> excludeBookies) {
- Set<BookieId> comprehensiveExclusionBookiesSet;
- if (enforceMinNumRacksPerWriteQuorum) {
- Set<BookieId> bookiesInDefaultRack = null;
- Set<Node> defaultRackLeaves = topology.getLeaves(getDefaultRack());
- for (Node node : defaultRackLeaves) {
- if (node instanceof BookieNode) {
- if (bookiesInDefaultRack == null) {
- bookiesInDefaultRack = new
HashSet<BookieId>(excludeBookies);
- }
- bookiesInDefaultRack.add(((BookieNode) node).getAddr());
- } else {
- LOG.error("found non-BookieNode: {} as leaf of
defaultrack: {}", node, getDefaultRack());
+ if (!enforceMinNumRacksPerWriteQuorum) {
+ return excludeBookies;
+ }
+
+ Set<BookieId> bookiesInDefaultRack = null;
+ Set<Node> defaultRackLeaves = topology.getLeaves(getDefaultRack());
+ for (Node node : defaultRackLeaves) {
+ if (node instanceof BookieNode) {
+ if (bookiesInDefaultRack == null) {
+ bookiesInDefaultRack = new HashSet<>();
}
- }
- if ((bookiesInDefaultRack == null) ||
bookiesInDefaultRack.isEmpty()) {
- comprehensiveExclusionBookiesSet = excludeBookies;
+ bookiesInDefaultRack.add(((BookieNode) node).getAddr());
} else {
- comprehensiveExclusionBookiesSet = new
HashSet<BookieId>(excludeBookies);
- comprehensiveExclusionBookiesSet.addAll(bookiesInDefaultRack);
- LOG.info("enforceMinNumRacksPerWriteQuorum is enabled, so
Excluding bookies of defaultRack: {}",
- bookiesInDefaultRack);
+ LOG.error("found non-BookieNode: {} as leaf of defaultrack:
{}", node, getDefaultRack());
}
+ }
+ if ((bookiesInDefaultRack == null) || bookiesInDefaultRack.isEmpty()) {
Review Comment:
The null check for `bookiesInDefaultRack` is redundant. Since
`bookiesInDefaultRack` is only instantiated when nodes are found (line 371), it
can only be null if no BookieNode was encountered. The `isEmpty()` check alone
would be sufficient after removing the null check, but the logic should be
restructured to check `bookiesInDefaultRack == null` separately from
`isEmpty()` for clarity, or initialize it to an empty set at declaration.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java:
##########
@@ -359,32 +359,31 @@ public void uninitalize() {
*/
protected Set<BookieId> addDefaultRackBookiesIfMinNumRacksIsEnforced(
Set<BookieId> excludeBookies) {
- Set<BookieId> comprehensiveExclusionBookiesSet;
- if (enforceMinNumRacksPerWriteQuorum) {
- Set<BookieId> bookiesInDefaultRack = null;
- Set<Node> defaultRackLeaves = topology.getLeaves(getDefaultRack());
- for (Node node : defaultRackLeaves) {
- if (node instanceof BookieNode) {
- if (bookiesInDefaultRack == null) {
- bookiesInDefaultRack = new
HashSet<BookieId>(excludeBookies);
- }
- bookiesInDefaultRack.add(((BookieNode) node).getAddr());
- } else {
- LOG.error("found non-BookieNode: {} as leaf of
defaultrack: {}", node, getDefaultRack());
+ if (!enforceMinNumRacksPerWriteQuorum) {
+ return excludeBookies;
+ }
+
+ Set<BookieId> bookiesInDefaultRack = null;
+ Set<Node> defaultRackLeaves = topology.getLeaves(getDefaultRack());
+ for (Node node : defaultRackLeaves) {
+ if (node instanceof BookieNode) {
+ if (bookiesInDefaultRack == null) {
+ bookiesInDefaultRack = new HashSet<>();
}
- }
- if ((bookiesInDefaultRack == null) ||
bookiesInDefaultRack.isEmpty()) {
- comprehensiveExclusionBookiesSet = excludeBookies;
+ bookiesInDefaultRack.add(((BookieNode) node).getAddr());
} else {
- comprehensiveExclusionBookiesSet = new
HashSet<BookieId>(excludeBookies);
- comprehensiveExclusionBookiesSet.addAll(bookiesInDefaultRack);
- LOG.info("enforceMinNumRacksPerWriteQuorum is enabled, so
Excluding bookies of defaultRack: {}",
- bookiesInDefaultRack);
+ LOG.error("found non-BookieNode: {} as leaf of defaultrack:
{}", node, getDefaultRack());
Review Comment:
Corrected spelling of 'defaultrack' to 'defaultRack'.
```suggestion
LOG.error("found non-BookieNode: {} as leaf of defaultRack:
{}", node, getDefaultRack());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
