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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
