[GitHub] [pulsar] hangc0276 commented on a change in pull request #10715: ZkIsolatedBookieEnsemblePlacementPolicy should use bookie zookeeper

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

hangc0276 commented on a change in pull request #10715:
URL: https://github.com/apache/pulsar/pull/10715#discussion_r644867291



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/PulsarClusterMetadataSetup.java
##########
@@ -224,10 +227,21 @@ public static void main(String[] args) throws Exception {
             initializer.initializeCluster(bkMetadataServiceUri.getUri(), arguments.numStreamStorageContainers);
         }
 
-        if (!localStore.exists(ZkBookieRackAffinityMapping.BOOKIE_INFO_ROOT_PATH).get()) {
-            createMetadataNode(localStore, ZkBookieRackAffinityMapping.BOOKIE_INFO_ROOT_PATH, "{}".getBytes());
+        // exist the existing-bk-metadata-service-uri or bookkeeper-metadata-service-uri,
+        // should create metadata on the bookkeeper side
+        if (existBk) {
+            String bkZKStr = ZkUtils.getBookieZkConnect(uriStr);
+            MetadataStoreExtended bookieStore = initMetadataStore(bkZKStr, arguments.zkSessionTimeoutMillis);

Review comment:
       the created bookieStore should close in the end.

##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -1022,6 +1025,28 @@ public ZooKeeper getZkClient() {
         return this.localZooKeeperConnectionProvider.getLocalZooKeeper();
     }
 
+    // get bookkeeper's zookeeper
+    public ZooKeeper getBookieZkClient() {

Review comment:
       the `bookieZkClient` will be change root to ip:port/ledgers? and the following operation such as `bookieZk().setData(ZkBookieRackAffinityMapping.BOOKIE_INFO_ROOT_PATH,...)` will set data to ip:port/ledgers/bookies path?

##########
File path: pulsar-zookeeper-utils/src/main/java/org/apache/pulsar/zookeeper/ZkUtils.java
##########
@@ -121,4 +124,45 @@ public static String getParentForPath(final String path) {
         final String parentPath = sb.toString();
         return (parentPath.length() == 0) ? null : parentPath;
     }
+
+
+    /**
+     * Returns the trimmed "/ledgers" string
+     *
+     * @param path connect path sting
+     * @return the trimmed "/ledgers" string
+     */
+    public static final String trimLedgersDefaultRootPath(String path) {
+        String rs = path;
+        if (StringUtils.endsWith(path, "/")) {
+            rs = StringUtils.substringBeforeLast(path, "/");
+        }
+        // defalut "/ledgers" need to trim
+        if (StringUtils.endsWith(path, LEDGERS_DEFAULT_ROOT_PATH)) {

Review comment:
       It shouldn't assume the ledger's root path is `/ledgers`, that's just default value in bookkeeper.conf, it maybe changed by bookkeeper.conf or broker.conf. 
   
   Last but not least, `ZkBookieRackAffinityMapping` will be used for bookkeeper client and bookkeeper auto recovery, it will call  `trimLedgersDefaultRootPath`, and it shouldn't just trim the default value.

##########
File path: pulsar-zookeeper-utils/src/test/java/org/apache/pulsar/zookeeper/ZookeeperServerTest.java
##########
@@ -41,18 +41,18 @@ public ZookeeperServerTest(int zkPort) throws IOException {
         if (!zkTmpDir.delete() || !zkTmpDir.mkdir()) {
             throw new IOException("Couldn't create zk directory " + zkTmpDir);
         }
+        // Allow all commands on ZK control port
+        System.setProperty("zookeeper.4lw.commands.whitelist", "*");

Review comment:
       +1




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