You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2022/12/23 18:42:04 UTC
[GitHub] [helix] rahulrane50 commented on a diff in pull request #2328: Multi Implementation and Test Cases
rahulrane50 commented on code in PR #2328:
URL: https://github.com/apache/helix/pull/2328#discussion_r1056554655
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/FederatedZkClient.java:
##########
@@ -477,10 +477,31 @@ public long getCreationTime(String path) {
return getZkClient(path).getCreationTime(path);
}
+ /**
+ * Executes ZkMulti on operations that are connected to the same Zk server.
+ * Will throw exception if any operation's server connection is different.
+ * @param ops
+ * @return
+ * @throws IllegalArgumentException
+ */
@Override
public List<OpResult> multi(Iterable<Op> ops) {
- throwUnsupportedOperationException();
- return null;
+ if (ops == null) {
+ throw new NullPointerException("ops must not be null.");
+ }
+ String opPath = null;
+ String opPathRealm = null;
+ for (Op op : ops) {
+ if (opPath == null) {
+ opPath = op.getPath();
+ opPathRealm = getZkRealm(op.getPath());
+ } else {
+ if (!opPathRealm.equals(getZkRealm(op.getPath()))){
+ throw new IllegalArgumentException("Cannot execute multi on ops of different realms!");
+ }
+ }
+ }
+ return getZkClient(opPath).multi(ops);
Review Comment:
also this getZkClient might fail if none of the ops in provided list have valid opPath right?
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/FederatedZkClient.java:
##########
@@ -477,10 +477,31 @@ public long getCreationTime(String path) {
return getZkClient(path).getCreationTime(path);
}
+ /**
+ * Executes ZkMulti on operations that are connected to the same Zk server.
+ * Will throw exception if any operation's server connection is different.
+ * @param ops
+ * @return
+ * @throws IllegalArgumentException
+ */
@Override
public List<OpResult> multi(Iterable<Op> ops) {
- throwUnsupportedOperationException();
- return null;
+ if (ops == null) {
+ throw new NullPointerException("ops must not be null.");
+ }
+ String opPath = null;
+ String opPathRealm = null;
+ for (Op op : ops) {
+ if (opPath == null) {
+ opPath = op.getPath();
+ opPathRealm = getZkRealm(op.getPath());
Review Comment:
Neat: we already got opPath on line 496, can we re-use it here?
##########
zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/RealmAwareZkClientTestBase.java:
##########
@@ -67,4 +76,127 @@ public void afterClass() {
_msdsServer.stopServer();
}
}
-}
+ /**
+ * Initialize requirement for testing multi support.
+ */
+ @Test
+ public void testMulti() {
+ // Create a connection config with a valid sharding key
+ RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder builder =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder();
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
+ builder.setZkRealmShardingKey(ZK_SHARDING_KEY_PREFIX).build();
+ try {
+ _realmAwareZkClient = new FederatedZkClient(connectionConfig,
+ new RealmAwareZkClient.RealmAwareZkClientConfig());
+ } catch (IllegalArgumentException e) {
+ Assert.fail("Invalid Sharding Key.");
+ } catch (Exception e) {
+ Assert.fail("Should not see any other types of Exceptions: " + e);
+ }
+ }
+
+ /**
+ * Test that zk multi works for create.
+ */
+ @Test(dependsOnMethods = "testMulti")
+ public void testMultiCreate() {
+ String test_name = "/test_multi_create";
+
+ //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
+ List<OpResult> opResults = _realmAwareZkClient.multi(ops);
+ Assert.assertTrue(opResults.get(0) instanceof OpResult.CreateResult);
+ Assert.assertTrue(opResults.get(1) instanceof OpResult.CreateResult);
+
+ cleanup();
+ }
+
+ /**
+ * Multi should be an all or nothing transaction. Creating correct
+ * paths and a singular bad one should all fail.
+ */
+ @Test(dependsOnMethods = "testMultiCreate")
Review Comment:
Curious, why does it depends on "testMultiCreate"?
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/FederatedZkClient.java:
##########
@@ -477,10 +477,31 @@ public long getCreationTime(String path) {
return getZkClient(path).getCreationTime(path);
}
+ /**
+ * Executes ZkMulti on operations that are connected to the same Zk server.
+ * Will throw exception if any operation's server connection is different.
+ * @param ops
+ * @return
+ * @throws IllegalArgumentException
+ */
@Override
public List<OpResult> multi(Iterable<Op> ops) {
- throwUnsupportedOperationException();
- return null;
+ if (ops == null) {
+ throw new NullPointerException("ops must not be null.");
+ }
+ String opPath = null;
+ String opPathRealm = null;
+ for (Op op : ops) {
Review Comment:
From my understanding of code, in this for loop you want to filter ops who have same ZkRealm. Just wondering if there could be case where first few ops in given ops list won't have any realm but later ones have it. In that case, here even those first few ops will be sent for "multi" operation right? Should we use any lambda filter method which filters out all ops with same ZkRealm.
--
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