[GitHub] [bookkeeper] hangc0276 commented on a diff in pull request #4013: Replace bookie support downgrade to replace bookie with itself.

["hangc0276 (via GitHub)" <gi...@apache.org>]

hangc0276 commented on code in PR #4013:
URL: https://github.com/apache/bookkeeper/pull/4013#discussion_r1250681779


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java:
##########
@@ -482,16 +490,33 @@ public PlacementResult<BookieId> replaceBookie(int ensembleSize, int writeQuorum
 
             if (LOG.isDebugEnabled()) {
                 LOG.debug("Try to choose a new bookie to replace {} from ensemble {}, excluding {}.",
-                    bookieToReplace, ensembleNodes, excludeNodes);
+                        bookieToReplace, ensembleNodes, excludeNodes);
             }
             // pick a candidate from same rack to replace
-            BookieNode candidate = selectFromNetworkLocation(
-                    bn.getNetworkLocation(),
-                    networkLocationsToBeExcluded,
-                    excludeNodes,
-                    TruePredicate.INSTANCE,
-                    EnsembleForReplacementWithNoConstraints.INSTANCE,
-                    !enforceMinNumRacksPerWriteQuorum);
+            BookieNode candidate;
+            try {
+                candidate = selectFromNetworkLocation(
+                        bn.getNetworkLocation(),
+                        networkLocationsToBeExcluded,
+                        excludeNodes,
+                        TruePredicate.INSTANCE,
+                        EnsembleForReplacementWithNoConstraints.INSTANCE,
+                        !enforceMinNumRacksPerWriteQuorum);
+            } catch (BKNotEnoughBookiesException e) {
+                if (downgradeToSelf) {
+                    candidate = knownBookies.get(bookieToReplace);

Review Comment:
   We need to ensure the bookie is writable. The knownBookies set contains all the bookies which are writable and read-only.



##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java:
##########
@@ -1061,14 +1061,15 @@ private Map<Integer, BookieId> getReplacementBookies(
             }
         }
         return getReplacementBookiesByIndexes(
-                lh, ensemble, bookieIndexesToRereplicate, Optional.of(bookiesToRereplicate));
+                lh, ensemble, bookieIndexesToRereplicate, Optional.of(bookiesToRereplicate), false);

Review Comment:
   Can we make it to `true`, so that we can remove all the added new parameters?



-- 
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: commits-unsubscribe@bookkeeper.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org