[GitHub] [helix] qqu0127 commented on a diff in pull request #2239: Adding Transactional support in Helix Zk Client

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

qqu0127 commented on code in PR #2239:
URL: https://github.com/apache/helix/pull/2239#discussion_r1007155700


##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/FederatedZkClient.java:
##########
@@ -479,8 +479,18 @@ public long getCreationTime(String path) {
 
   @Override
   public List<OpResult> multi(Iterable<Op> ops) {
-    throwUnsupportedOperationException();
-    return null;
+    if (ops == null) {
+      throw new NullPointerException("ops must not be null.");
+    }
+    String op_path = ops.iterator().next().getPath();
+    // Check whether all ops are connected to the same ZK server. If any differ, multi can't be run.
+    for (Op op : ops) {
+      if (!getZkRealm(op_path).equals(getZkRealm(op.getPath()))){
+        throwUnsupportedOperationException();

Review Comment:
   We should better throw IllegalArgumentException



##########
zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/RealmAwareZkClientFactoryTestBase.java:
##########
@@ -44,6 +51,7 @@ public abstract class RealmAwareZkClientFactoryTestBase extends RealmAwareZkClie
   protected RealmAwareZkClientFactory _realmAwareZkClientFactory;
   protected RealmAwareZkClient _realmAwareZkClient;
   private static final ZNRecord DUMMY_RECORD = new ZNRecord("DummyRecord");
+  protected String PARENT_PATH;

Review Comment:
   Can this be a static final constant? Let's try to put it in its subclasses, not base class.



##########
zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestFederatedZkClient.java:
##########
@@ -662,4 +654,124 @@ public void testClose() {
       Assert.assertEquals(ex.getMessage(), "FederatedZkClient is closed!");
     }
   }
+
+  /**
+   * Test that zk multi works for op.create.
+   */
+  @Test
+  public void testMultiCreate() {
+    String test_name = "/test_multi_create";
+    _realmAwareZkClient.createPersistent(ZK_SHARDING_KEY_PREFIX);
+
+    //Create Nodes
+    List<Op> ops = Arrays.asList(
+            Op.create(PARENT_PATH, new byte[0],
+                    ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT),
+            Op.create(PARENT_PATH + test_name, new byte[0],
+                    ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT));
+
+    //Execute transactional support on operations and verify they were run
+    try {
+      List<OpResult> opResults = _realmAwareZkClient.multi(ops);
+      Assert.assertTrue(opResults.get(0) instanceof OpResult.CreateResult);
+      Assert.assertTrue(opResults.get(1) instanceof OpResult.CreateResult);
+      cleanup();
+    } catch (UnsupportedOperationException e) {
+      Assert.fail("Zk Multi Not Supported!");
+    }

Review Comment:
   Is this catch necessary?



##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/FederatedZkClient.java:
##########
@@ -479,8 +479,18 @@ public long getCreationTime(String path) {
 
   @Override
   public List<OpResult> multi(Iterable<Op> ops) {
-    throwUnsupportedOperationException();
-    return null;
+    if (ops == null) {
+      throw new NullPointerException("ops must not be null.");
+    }
+    String op_path = ops.iterator().next().getPath();

Review Comment:
   Two things:
   Let's unify the variable naming with camelcase. 
   Can we embed the zk path check in the for-loop? There is a small overhead of instantiating this extra iterator.



-- 
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: reviews-unsubscribe@helix.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org