[GitHub] [pulsar] sijie commented on a change in pull request #8961: Fixes the recovery not respect to the isolation group settings

[GitBox <gi...@apache.org>]

sijie commented on a change in pull request #8961:
URL: https://github.com/apache/pulsar/pull/8961#discussion_r544828698



##########
File path: pulsar-zookeeper-utils/src/main/java/org/apache/pulsar/zookeeper/ZkIsolatedBookieEnsemblePlacementPolicy.java
##########
@@ -147,6 +157,11 @@ private ZooKeeperCache getAndSetZkCache(Configuration conf) {
             Map<String, byte[]> customMetadata, List<BookieId> currentEnsemble,
             BookieId bookieToReplace, Set<BookieId> excludeBookies)
             throws BKNotEnoughBookiesException {
+        // parse the ensemble placement policy from the custom metadata, if it is present, we will apply it to
+        // the isolation groups for filtering the bookies.
+        Optional<EnsemblePlacementPolicyConfig> ensemblePlacementPolicyConfig =
+            getEnsemblePlacementPolicyConfig(customMetadata);
+        ensemblePlacementPolicyConfig.ifPresent(this::fillIsolationGroupWithEnsemblePlacementPolicyConfig);

Review comment:
       The change here has concurrent access issues. Because it resets the primary and secondary isolation groups. It will cause replacing bookies using the wrong group. 
   
   Also, I don't think the logic here is correct.
   
   I think we should record the `group` information in the custom metadata. when replacing a bookie, we need to lookup the bookies in the same group. If it is not an easy change to make here, we should try to build a new bookie ensemble placement policy to achieve this goal and can be used for autorecovery.
   
   




----------------------------------------------------------------
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.

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