[GitHub] [flink] zentol commented on a change in pull request #15893: [FLINK-22636] Group job specific zNodes under /jobs zNode

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

zentol commented on a change in pull request #15893:
URL: https://github.com/apache/flink/pull/15893#discussion_r630771902



##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/leaderretrieval/ZooKeeperLeaderRetrievalDriver.java
##########
@@ -188,6 +190,15 @@ public void unhandledError(String s, Throwable throwable) {
 
     @Override
     public String toString() {
-        return "ZookeeperLeaderRetrievalDriver{" + "retrievalPath='" + retrievalPath + '\'' + '}';
+        return "ZookeeperLeaderRetrievalDriver{"
+                + "retrievalPath='"

Review comment:
       nit: outdated

##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/util/ZooKeeperUtils.java
##########
@@ -493,14 +522,17 @@ public static String generateZookeeperPath(String root, String namespace) {
      */
     public static CuratorFramework useNamespaceAndEnsurePath(
             final CuratorFramework client, final String path) throws Exception {
-        Preconditions.checkNotNull(client, "client must not be null");
-        Preconditions.checkNotNull(path, "path must not be null");
+        checkNotNull(client, "client must not be null");
+        checkNotNull(path, "path must not be null");
 
         // Ensure that the checkpoints path exists
         client.newNamespaceAwareEnsurePath(path).ensure(client.getZookeeperClient());
 
         // All operations will have the path as root
-        return client.usingNamespace(generateZookeeperPath(client.getNamespace(), path));
+        return client.usingNamespace(
+                // Curator prepends a '/' manually and throws an Exception if the
+                // namespace starts with a '/'.
+                generateZookeeperPath(client.getNamespace(), path).substring(1));

Review comment:
       maybe add a check that the first character is really a `/`

##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/util/ZooKeeperUtils.java
##########
@@ -466,20 +475,40 @@ public static ZooKeeperCheckpointIDCounter createCheckpointIDCounter(
                 prefix);
     }
 
+    /** Creates a ZooKeeper path of the form "/root/namespace". */
     public static String generateZookeeperPath(String root, String namespace) {
-        if (!namespace.startsWith("/")) {
-            namespace = '/' + namespace;
-        }
+        final String result =
+                Stream.of(root, namespace)
+                        .map(ZooKeeperUtils::trimSlashes)
+                        .filter(s -> !s.isEmpty())
+                        .collect(Collectors.joining("/"));

Review comment:
       nit:
   ```suggestion
                           .collect(Collectors.joining("/", "/", ""));
   ```




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