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/11/21 18:50:53 UTC
[GitHub] [helix] Marcosrico opened a new pull request, #2239: [WIP] Rearranging Tests to Accommodate ZkMulti
Marcosrico opened a new pull request, #2239:
URL: https://github.com/apache/helix/pull/2239
To test implementation of FederatedZkClient Multi, tests must be written. Mainly rearranged current test base for cleaner code and added specific test for FederatedZkClient multi support.
### Issues
N.A.
### Tests
Solely rearranged tests under `/multizk` and combined similar code to clean up the tests. Wrote separate test to test future implementation of FederatedZkClient ZkMulti in `testMultiInMultiZk`. Currently FederatedZkClient ZkMulti is not supported.
Current test failure is known issue: https://github.com/apache/helix/issues/2286
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
--
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
[GitHub] [helix] xyuanlu commented on a diff in pull request #2239: Adding Transactional support in Helix Zk Client
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on code in PR #2239:
URL: https://github.com/apache/helix/pull/2239#discussion_r1007180484
##########
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.
+ */
Review Comment:
I think since this test class already extends RealmAwareZkClientTestBase, the following tests should already be included.
We could add test cases that are specific to federatedZKClient implementation of multi. For example, try to create multiple ZNode that do not belongs to one realm and expect a failure.
--
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
[GitHub] [helix] xyuanlu commented on a diff in pull request #2239: Adding Transactional support in Helix Zk Client
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on code in PR #2239:
URL: https://github.com/apache/helix/pull/2239#discussion_r1007149317
##########
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:
I think we can call `getZkRealm(op_path)` here and store the realm to same some map lookup.
--
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
[GitHub] [helix] Marcosrico closed pull request #2239: [WIP] Rearranging Tests to Accommodate ZkMulti
Posted by GitBox <gi...@apache.org>.
Marcosrico closed pull request #2239: [WIP] Rearranging Tests to Accommodate ZkMulti
URL: https://github.com/apache/helix/pull/2239
--
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
[GitHub] [helix] Marcosrico closed pull request #2239: [WIP] Rearranging Tests to Accommodate ZkMulti
Posted by GitBox <gi...@apache.org>.
Marcosrico closed pull request #2239: [WIP] Rearranging Tests to Accommodate ZkMulti
URL: https://github.com/apache/helix/pull/2239
--
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
[GitHub] [helix] Marcosrico commented on a diff in pull request #2239: Adding Transactional support in Helix Zk Client
Posted by GitBox <gi...@apache.org>.
Marcosrico commented on code in PR #2239:
URL: https://github.com/apache/helix/pull/2239#discussion_r1023338622
##########
helix-core/src/test/java/org/apache/helix/integration/multizk/MultiZkTestBase.java:
##########
@@ -0,0 +1,150 @@
+package org.apache.helix.integration.multizk;
Review Comment:
Copied Before and After class from pre-existing tests in MultiZkConnectionConfig to avoid code repetition for the other test cases.
--
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
[GitHub] [helix] xyuanlu commented on a diff in pull request #2239: Adding Transactional support in Helix Zk Client
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on code in PR #2239:
URL: https://github.com/apache/helix/pull/2239#discussion_r1007150876
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/FederatedZkClient.java:
##########
@@ -479,8 +479,18 @@ public long getCreationTime(String path) {
Review Comment:
Please add API description here and explain where are supported in federatedZK client and in witch cases we throw exception.
--
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
[GitHub] [helix] qqu0127 commented on a diff in pull request #2239: Adding Transactional support in Helix Zk Client
Posted by 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
[GitHub] [helix] qqu0127 commented on a diff in pull request #2239: Adding Transactional support in Helix Zk Client
Posted by GitBox <gi...@apache.org>.
qqu0127 commented on code in PR #2239:
URL: https://github.com/apache/helix/pull/2239#discussion_r1024620760
##########
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();
Review Comment:
Some exception message will be great, to indicate not all operations are using the same realm.
##########
helix-core/src/test/java/org/apache/helix/integration/multizk/TestMultiInMultiZk.java:
##########
@@ -0,0 +1,74 @@
+package org.apache.helix.integration.multizk;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.apache.helix.zookeeper.constant.RoutingDataReaderType;
+import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer;
+import org.apache.helix.zookeeper.impl.client.FederatedZkClient;
+import org.apache.helix.zookeeper.routing.RoutingDataManager;
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.Op;
+import org.apache.zookeeper.ZooDefs;
+import org.testng.Assert;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+import java.util.*;
+
+/**
+ * This class test multi implementation in FederatedZkClient. Extends MultiZkTestBase as the test require a multi zk
+ * server setup.
+ */
+public class TestMultiInMultiZk extends MultiZkTestBase {
+
+ @BeforeClass
+ public void beforeClass() throws Exception {
+ super.beforeClass();
+ // Routing data may be set by other tests using the same endpoint; reset() for good measure
+ RoutingDataManager.getInstance().reset();
+ // Create a FederatedZkClient for admin work
+
+ try {
+ _zkClient =
+ new FederatedZkClient(new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
+ .setRoutingDataSourceEndpoint(_msdsEndpoint + "," + ZK_PREFIX + ZK_START_PORT)
+ .setRoutingDataSourceType(RoutingDataReaderType.HTTP_ZK_FALLBACK.name()).build(),
+ new RealmAwareZkClient.RealmAwareZkClientConfig());
+ _zkClient.setZkSerializer(new ZNRecordSerializer());
+ } catch (Exception ex) {
+ for (StackTraceElement elm : ex.getStackTrace()) {
+ System.out.println(elm);
+ }
+ }
+ System.out.println("end start");
+ }
+
+ @AfterClass
+ public void afterClass() throws Exception {
+ super.afterClass();
+ }
Review Comment:
is this necessary?
##########
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()))){
Review Comment:
else if can be combined
##########
helix-core/src/test/java/org/apache/helix/integration/multizk/MultiZkTestBase.java:
##########
@@ -0,0 +1,150 @@
+package org.apache.helix.integration.multizk;
Review Comment:
please add apache license, thanks
--
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