Fix invalid rack name cause bookie join rack failed (#16845)

apache · Aug 3, 2022 · 0c14efb · 0c14efb
1 parent 8c25d25
commit 0c14efb
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 9 deletions.
diff --git a/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdBookies.java b/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdBookies.java
@@ -18,11 +18,12 @@
  */
 package org.apache.pulsar.admin.cli;
 
-import org.apache.pulsar.client.admin.PulsarAdmin;
-import org.apache.pulsar.common.policies.data.BookieInfo;
-
 import com.beust.jcommander.Parameter;
 import com.beust.jcommander.Parameters;
+import com.google.common.base.Strings;
+import lombok.NonNull;
+import org.apache.pulsar.client.admin.PulsarAdmin;
+import org.apache.pulsar.common.policies.data.BookieInfo;
 
 @Parameters(commandDescription = "Operations about bookies rack placement")
 public class CmdBookies extends CmdBase {
@@ -62,6 +63,8 @@ void run() throws Exception {
 
     @Parameters(commandDescription = "Updates the rack placement information for a specific bookie in the cluster (note. bookie address format:`address:port`)")
     private class UpdateBookie extends CliCommand {
+        private static final String PATH_SEPARATOR = "/";
+
         @Parameter(names = { "-g", "--group" }, description = "Bookie group name", required = false)
         private String group = "default";
 
@@ -76,8 +79,16 @@ private class UpdateBookie extends CliCommand {
 
         @Override
         void run() throws Exception {
+            checkArgument(!Strings.isNullOrEmpty(bookieRack) && !bookieRack.trim().equals(PATH_SEPARATOR),
+                    "rack name is invalid, it should not be null, empty or '/'");
             admin.bookies().updateBookieRackInfo(bookieAddress, group, new BookieInfo(bookieRack, bookieHost));
         }
+
+        private void checkArgument(boolean expression, @NonNull Object errorMessage) {
+            if (!expression) {
+                throw new IllegalArgumentException(String.valueOf(errorMessage));
+            }
+        }
     }
 
     public CmdBookies(PulsarAdmin admin) {

diff --git a/...ookeeper-utils/src/main/java/org/apache/pulsar/zookeeper/ZkBookieRackAffinityMapping.java b/...ookeeper-utils/src/main/java/org/apache/pulsar/zookeeper/ZkBookieRackAffinityMapping.java
@@ -19,7 +19,7 @@
 package org.apache.pulsar.zookeeper;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
-
+import com.google.common.base.Strings;
 import java.net.InetAddress;
 import java.net.UnknownHostException;
 import java.util.ArrayList;
@@ -28,7 +28,6 @@
 import java.util.Map;
 import java.util.Optional;
 import java.util.concurrent.TimeUnit;
-
 import org.apache.bookkeeper.client.ITopologyAwareEnsemblePlacementPolicy;
 import org.apache.bookkeeper.client.RackChangeNotifier;
 import org.apache.bookkeeper.conf.ClientConfiguration;
@@ -197,7 +196,9 @@ private String getRack(String bookieAddress) {
             }
         }
 
-        if (bi != null) {
+        if (bi != null
+                && !Strings.isNullOrEmpty(bi.getRack())
+                && !bi.getRack().trim().equals("/")) {
             String rack = bi.getRack();
             if (!rack.startsWith("/")) {
                 rack = "/" + rack;

diff --git a/...eper-utils/src/test/java/org/apache/pulsar/zookeeper/ZkBookieRackAffinityMappingTest.java b/...eper-utils/src/test/java/org/apache/pulsar/zookeeper/ZkBookieRackAffinityMappingTest.java
@@ -19,14 +19,12 @@
 package org.apache.pulsar.zookeeper;
 
 import static org.testng.Assert.assertEquals;
-
+import static org.testng.AssertJUnit.assertNull;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.collect.Lists;
-
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-
 import org.apache.bookkeeper.conf.ClientConfiguration;
 import org.apache.bookkeeper.net.BookieSocketAddress;
 import org.apache.bookkeeper.util.ZkUtils;
@@ -101,6 +99,29 @@ public void testBasic() throws Exception {
         localZkc.delete(ZkBookieRackAffinityMapping.BOOKIE_INFO_ROOT_PATH, -1);
     }
 
+    @Test
+    public void testInvalidRackName() throws Exception {
+        String data = "{\"group1\": {\"" + BOOKIE1
+                + "\": {\"rack\": \"/\", \"hostname\": \"bookie1.example.com\"}, \"" + BOOKIE2
+                + "\": {\"rack\": \"\", \"hostname\": \"bookie2.example.com\"}}}";
+
+        ZkUtils.createFullPathOptimistic(localZkc, ZkBookieRackAffinityMapping.BOOKIE_INFO_ROOT_PATH, data.getBytes(),
+                ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+
+        // Case1: ZKCache is given
+        ZkBookieRackAffinityMapping mapping1 = new ZkBookieRackAffinityMapping();
+        ClientConfiguration bkClientConf1 = new ClientConfiguration();
+        bkClientConf1.setProperty(ZooKeeperCache.ZK_CACHE_INSTANCE, new ZooKeeperCache("test", localZkc, 30) {
+        });
+        mapping1.setConf(bkClientConf1);
+        List<String> racks1 = mapping1
+                .resolve(Lists.newArrayList(BOOKIE1.getHostName(), BOOKIE2.getHostName(), BOOKIE3.getHostName()));
+
+        assertNull(racks1.get(0));
+        assertNull(racks1.get(1));
+        assertNull(racks1.get(2));
+    }
+
     @Test
     public void testNoBookieInfo() throws Exception {
         ZkBookieRackAffinityMapping mapping = new ZkBookieRackAffinityMapping();