[GitHub] [helix] narendly merged pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
narendly merged pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders URL: https://github.com/apache/helix/pull/899 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang merged pull request #904: Fix the concurrent modification error happens during the HelixManager initHandlers() call
jiajunwang merged pull request #904: Fix the concurrent modification error happens during the HelixManager initHandlers() call URL: https://github.com/apache/helix/pull/904 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on issue #904: Fix the concurrent modification error happens during the HelixManager initHandlers() call
jiajunwang commented on issue #904: Fix the concurrent modification error happens during the HelixManager initHandlers() call URL: https://github.com/apache/helix/pull/904#issuecomment-601972438 This PR is ready to be merged, approved by @pkuwm 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #904: Fix the concurrent modification error happens during the HelixManager initHandlers() call
jiajunwang commented on a change in pull request #904: Fix the concurrent
modification error happens during the HelixManager initHandlers() call
URL: https://github.com/apache/helix/pull/904#discussion_r395946837
##
File path:
helix-core/src/test/java/org/apache/helix/manager/zk/TestHandleSession.java
##
@@ -466,6 +468,79 @@ public void testSessionExpiredWhenResetHandlers() throws
Exception {
deleteCluster(clusterName);
}
+ @Test
+ public void testConcurrentInitCallbackHandlers() throws Exception {
+final String clusterName =
+CLUSTER_PREFIX + "_" + _className + "_" +
TestHelper.getTestMethodName();
+TestHelper.setupEmptyCluster(_gZkClient, clusterName);
+final String spectatorName = "testConcurrentHandlerChangeSpectator";
+try {
+ BlockingHandleNewSessionZkHelixManager helixManager =
+ new BlockingHandleNewSessionZkHelixManager(clusterName,
spectatorName,
+ InstanceType.SPECTATOR, _gZkClient.getServers());
+ helixManager.connect();
+ // Add two mock listeners that will add more callback handlers while
handling INIT or CALLBACK event.
+ // Note that we have to test with 2 separate listeners so one of them
has a chance to fail if
+ // there is a concurrent modification exception.
+ helixManager.addLiveInstanceChangeListener(
+ (LiveInstanceChangeListener) (liveInstances, changeContext) -> {
+if (changeContext.getType() != NotificationContext.Type.FINALIZE) {
+ for (LiveInstance liveInstance : liveInstances) {
+if (liveInstance.getInstanceName().equals("localhost_1")) {
+ try {
+helixManager.addCurrentStateChangeListener(
+(CurrentStateChangeListener) (instanceName,
statesInfo, currentStateChangeContext) -> {
+ // empty callback
+}, liveInstance.getInstanceName(),
liveInstance.getEphemeralOwner());
+ } catch (Exception e) {
+throw new HelixException(
+"Unexpected exception in the
testConcurrentHandlerProcessing.", e);
+ }
+}
+ }
+}
+ });
+ helixManager.addLiveInstanceChangeListener(
+ (LiveInstanceChangeListener) (liveInstances, changeContext) -> {
+if (changeContext.getType() != NotificationContext.Type.FINALIZE) {
+ for (LiveInstance liveInstance : liveInstances) {
+if (liveInstance.getInstanceName().equals("localhost_2")) {
+ try {
+helixManager.addCurrentStateChangeListener(
+(CurrentStateChangeListener) (instanceName,
statesInfo, currentStateChangeContext) -> {
+ // empty callback
+}, liveInstance.getInstanceName(),
liveInstance.getEphemeralOwner());
+ } catch (Exception e) {
+throw new HelixException(
+"Unexpected exception in the
testConcurrentHandlerProcessing.", e);
+ }
+}
+ }
+}
+ });
+ // Session expire will trigger all callbacks to be init. And the
injected liveInstance
+ // listener will trigger more callbackhandlers to be registered during
the init process.
+ ZkTestHelper.asyncExpireSession(helixManager.getZkClient());
+ // Create mock live instance znodes to trigger the internal callback
handling logic which will
+ // modify the handler list.
+ setupLiveInstances(clusterName, new int[] { 1, 2 });
+ // Start new session handling so the manager will call the initHandler()
for initializing all
+ // existing handlers.
+ helixManager.proceedNewSessionHandling();
+ // Ensure the new session has been processed.
+ TestHelper.verify(() -> helixManager.getHandleNewSessionEndTime() != 0,
3000);
+ // Verify that both new mock current state callback handlers have been
initialized normally.
+ // Note that if there is concurrent modification that cause errors, one
of the callback will
+ // not be initialized normally.
+ for (CallbackHandler handler : helixManager.getHandlers()) {
+Assert.assertTrue(handler.isReady(),
+"CallbackHandler is not initialized as expected. It might be
caused by a ConcurrentModificationException");
Review comment:
This is done by the auto format.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe,
[GitHub] [helix] pkuwm commented on a change in pull request #904: Fix the concurrent modification error happens during the HelixManager initHandlers() call
pkuwm commented on a change in pull request #904: Fix the concurrent
modification error happens during the HelixManager initHandlers() call
URL: https://github.com/apache/helix/pull/904#discussion_r395929866
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##
@@ -1004,7 +1004,10 @@ void waitUntilConnected() {
void initHandlers(List handlers) {
synchronized (this) {
if (handlers != null) {
-for (CallbackHandler handler : handlers) {
+// get a copy of the list and iterate over the copy list
+// in case handler.init() modify the original handler list
Review comment:
Nit: modify -> modifies
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #904: Fix the concurrent modification error happens during the HelixManager initHandlers() call
pkuwm commented on a change in pull request #904: Fix the concurrent
modification error happens during the HelixManager initHandlers() call
URL: https://github.com/apache/helix/pull/904#discussion_r395940851
##
File path:
helix-core/src/test/java/org/apache/helix/manager/zk/TestHandleSession.java
##
@@ -466,6 +468,79 @@ public void testSessionExpiredWhenResetHandlers() throws
Exception {
deleteCluster(clusterName);
}
+ @Test
+ public void testConcurrentInitCallbackHandlers() throws Exception {
+final String clusterName =
+CLUSTER_PREFIX + "_" + _className + "_" +
TestHelper.getTestMethodName();
+TestHelper.setupEmptyCluster(_gZkClient, clusterName);
+final String spectatorName = "testConcurrentHandlerChangeSpectator";
+try {
+ BlockingHandleNewSessionZkHelixManager helixManager =
+ new BlockingHandleNewSessionZkHelixManager(clusterName,
spectatorName,
+ InstanceType.SPECTATOR, _gZkClient.getServers());
+ helixManager.connect();
+ // Add two mock listeners that will add more callback handlers while
handling INIT or CALLBACK event.
+ // Note that we have to test with 2 separate listeners so one of them
has a chance to fail if
+ // there is a concurrent modification exception.
+ helixManager.addLiveInstanceChangeListener(
+ (LiveInstanceChangeListener) (liveInstances, changeContext) -> {
+if (changeContext.getType() != NotificationContext.Type.FINALIZE) {
+ for (LiveInstance liveInstance : liveInstances) {
+if (liveInstance.getInstanceName().equals("localhost_1")) {
+ try {
+helixManager.addCurrentStateChangeListener(
+(CurrentStateChangeListener) (instanceName,
statesInfo, currentStateChangeContext) -> {
+ // empty callback
+}, liveInstance.getInstanceName(),
liveInstance.getEphemeralOwner());
+ } catch (Exception e) {
+throw new HelixException(
+"Unexpected exception in the
testConcurrentHandlerProcessing.", e);
+ }
+}
+ }
+}
+ });
+ helixManager.addLiveInstanceChangeListener(
+ (LiveInstanceChangeListener) (liveInstances, changeContext) -> {
+if (changeContext.getType() != NotificationContext.Type.FINALIZE) {
+ for (LiveInstance liveInstance : liveInstances) {
+if (liveInstance.getInstanceName().equals("localhost_2")) {
+ try {
+helixManager.addCurrentStateChangeListener(
+(CurrentStateChangeListener) (instanceName,
statesInfo, currentStateChangeContext) -> {
+ // empty callback
+}, liveInstance.getInstanceName(),
liveInstance.getEphemeralOwner());
+ } catch (Exception e) {
+throw new HelixException(
+"Unexpected exception in the
testConcurrentHandlerProcessing.", e);
+ }
+}
+ }
+}
+ });
+ // Session expire will trigger all callbacks to be init. And the
injected liveInstance
+ // listener will trigger more callbackhandlers to be registered during
the init process.
+ ZkTestHelper.asyncExpireSession(helixManager.getZkClient());
+ // Create mock live instance znodes to trigger the internal callback
handling logic which will
+ // modify the handler list.
+ setupLiveInstances(clusterName, new int[] { 1, 2 });
+ // Start new session handling so the manager will call the initHandler()
for initializing all
+ // existing handlers.
+ helixManager.proceedNewSessionHandling();
+ // Ensure the new session has been processed.
+ TestHelper.verify(() -> helixManager.getHandleNewSessionEndTime() != 0,
3000);
+ // Verify that both new mock current state callback handlers have been
initialized normally.
+ // Note that if there is concurrent modification that cause errors, one
of the callback will
+ // not be initialized normally.
+ for (CallbackHandler handler : helixManager.getHandlers()) {
+Assert.assertTrue(handler.isReady(),
+"CallbackHandler is not initialized as expected. It might be
caused by a ConcurrentModificationException");
Review comment:
Nit, wrap this long line by concatenation?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To
[GitHub] [helix] pkuwm commented on a change in pull request #904: Fix the concurrent modification error happens during the HelixManager initHandlers() call
pkuwm commented on a change in pull request #904: Fix the concurrent
modification error happens during the HelixManager initHandlers() call
URL: https://github.com/apache/helix/pull/904#discussion_r395940379
##
File path:
helix-core/src/test/java/org/apache/helix/manager/zk/TestHandleSession.java
##
@@ -466,6 +468,79 @@ public void testSessionExpiredWhenResetHandlers() throws
Exception {
deleteCluster(clusterName);
}
+ @Test
+ public void testConcurrentInitCallbackHandlers() throws Exception {
+final String clusterName =
+CLUSTER_PREFIX + "_" + _className + "_" +
TestHelper.getTestMethodName();
+TestHelper.setupEmptyCluster(_gZkClient, clusterName);
+final String spectatorName = "testConcurrentHandlerChangeSpectator";
+try {
+ BlockingHandleNewSessionZkHelixManager helixManager =
+ new BlockingHandleNewSessionZkHelixManager(clusterName,
spectatorName,
+ InstanceType.SPECTATOR, _gZkClient.getServers());
+ helixManager.connect();
+ // Add two mock listeners that will add more callback handlers while
handling INIT or CALLBACK event.
+ // Note that we have to test with 2 separate listeners so one of them
has a chance to fail if
+ // there is a concurrent modification exception.
+ helixManager.addLiveInstanceChangeListener(
+ (LiveInstanceChangeListener) (liveInstances, changeContext) -> {
+if (changeContext.getType() != NotificationContext.Type.FINALIZE) {
+ for (LiveInstance liveInstance : liveInstances) {
+if (liveInstance.getInstanceName().equals("localhost_1")) {
+ try {
+helixManager.addCurrentStateChangeListener(
+(CurrentStateChangeListener) (instanceName,
statesInfo, currentStateChangeContext) -> {
+ // empty callback
+}, liveInstance.getInstanceName(),
liveInstance.getEphemeralOwner());
+ } catch (Exception e) {
+throw new HelixException(
+"Unexpected exception in the
testConcurrentHandlerProcessing.", e);
Review comment:
testConcurrentHandlerProcessing -> testConcurrentInitCallbackHandlers.
May be better to just `getTestMethodName()`.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #904: Fix the concurrent modification error happens during the HelixManager initHandlers() call
pkuwm commented on a change in pull request #904: Fix the concurrent
modification error happens during the HelixManager initHandlers() call
URL: https://github.com/apache/helix/pull/904#discussion_r395940156
##
File path:
helix-core/src/test/java/org/apache/helix/manager/zk/TestHandleSession.java
##
@@ -466,6 +468,79 @@ public void testSessionExpiredWhenResetHandlers() throws
Exception {
deleteCluster(clusterName);
}
+ @Test
+ public void testConcurrentInitCallbackHandlers() throws Exception {
+final String clusterName =
+CLUSTER_PREFIX + "_" + _className + "_" +
TestHelper.getTestMethodName();
+TestHelper.setupEmptyCluster(_gZkClient, clusterName);
+final String spectatorName = "testConcurrentHandlerChangeSpectator";
+try {
+ BlockingHandleNewSessionZkHelixManager helixManager =
+ new BlockingHandleNewSessionZkHelixManager(clusterName,
spectatorName,
+ InstanceType.SPECTATOR, _gZkClient.getServers());
+ helixManager.connect();
+ // Add two mock listeners that will add more callback handlers while
handling INIT or CALLBACK event.
+ // Note that we have to test with 2 separate listeners so one of them
has a chance to fail if
+ // there is a concurrent modification exception.
+ helixManager.addLiveInstanceChangeListener(
+ (LiveInstanceChangeListener) (liveInstances, changeContext) -> {
+if (changeContext.getType() != NotificationContext.Type.FINALIZE) {
+ for (LiveInstance liveInstance : liveInstances) {
+if (liveInstance.getInstanceName().equals("localhost_1")) {
+ try {
+helixManager.addCurrentStateChangeListener(
+(CurrentStateChangeListener) (instanceName,
statesInfo, currentStateChangeContext) -> {
+ // empty callback
+}, liveInstance.getInstanceName(),
liveInstance.getEphemeralOwner());
+ } catch (Exception e) {
+throw new HelixException(
+"Unexpected exception in the
testConcurrentHandlerProcessing.", e);
+ }
+}
+ }
+}
+ });
+ helixManager.addLiveInstanceChangeListener(
+ (LiveInstanceChangeListener) (liveInstances, changeContext) -> {
+if (changeContext.getType() != NotificationContext.Type.FINALIZE) {
+ for (LiveInstance liveInstance : liveInstances) {
+if (liveInstance.getInstanceName().equals("localhost_2")) {
+ try {
+helixManager.addCurrentStateChangeListener(
+(CurrentStateChangeListener) (instanceName,
statesInfo, currentStateChangeContext) -> {
+ // empty callback
+}, liveInstance.getInstanceName(),
liveInstance.getEphemeralOwner());
+ } catch (Exception e) {
+throw new HelixException(
+"Unexpected exception in the
testConcurrentHandlerProcessing.", e);
Review comment:
testConcurrentHandlerProcessing -> testConcurrentInitCallbackHandlers
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #904: Fix the concurrent modification error happens during the HelixManager initHandlers() call
pkuwm commented on a change in pull request #904: Fix the concurrent
modification error happens during the HelixManager initHandlers() call
URL: https://github.com/apache/helix/pull/904#discussion_r395941462
##
File path:
helix-core/src/test/java/org/apache/helix/manager/zk/TestHandleSession.java
##
@@ -466,6 +468,79 @@ public void testSessionExpiredWhenResetHandlers() throws
Exception {
deleteCluster(clusterName);
}
+ @Test
+ public void testConcurrentInitCallbackHandlers() throws Exception {
+final String clusterName =
+CLUSTER_PREFIX + "_" + _className + "_" +
TestHelper.getTestMethodName();
+TestHelper.setupEmptyCluster(_gZkClient, clusterName);
+final String spectatorName = "testConcurrentHandlerChangeSpectator";
+try {
+ BlockingHandleNewSessionZkHelixManager helixManager =
+ new BlockingHandleNewSessionZkHelixManager(clusterName,
spectatorName,
+ InstanceType.SPECTATOR, _gZkClient.getServers());
+ helixManager.connect();
+ // Add two mock listeners that will add more callback handlers while
handling INIT or CALLBACK event.
+ // Note that we have to test with 2 separate listeners so one of them
has a chance to fail if
+ // there is a concurrent modification exception.
+ helixManager.addLiveInstanceChangeListener(
+ (LiveInstanceChangeListener) (liveInstances, changeContext) -> {
+if (changeContext.getType() != NotificationContext.Type.FINALIZE) {
+ for (LiveInstance liveInstance : liveInstances) {
+if (liveInstance.getInstanceName().equals("localhost_1")) {
+ try {
+helixManager.addCurrentStateChangeListener(
+(CurrentStateChangeListener) (instanceName,
statesInfo, currentStateChangeContext) -> {
+ // empty callback
+}, liveInstance.getInstanceName(),
liveInstance.getEphemeralOwner());
+ } catch (Exception e) {
+throw new HelixException(
+"Unexpected exception in the
testConcurrentHandlerProcessing.", e);
+ }
+}
+ }
+}
Review comment:
Can we wrap this block into a private method to avoid the duplicate code in
these 2 listeners?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] alirezazamani closed issue #833: TODO: Add routing table provider for customized view
alirezazamani closed issue #833: TODO: Add routing table provider for customized view URL: https://github.com/apache/helix/issues/833 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang opened a new pull request #904: Fix the concurrent modification error happens during the HelixManager initHandlers() call
jiajunwang opened a new pull request #904: Fix the concurrent modification
error happens during the HelixManager initHandlers() call
URL: https://github.com/apache/helix/pull/904
### Issues
- [ ] My PR addresses the following Helix issues and references them in the
PR description:
#903
### Description
- [ ] Here are some details about my PR, including screenshots of any UI
changes:
Fix the concurrent modification error that happens during the HelixManager
initHandlers() call.
### Tests
- [ ] The following tests are written for this issue:
TestHandleSession.testConcurrentInitCallbackHandlers()
- [ ] The following is the result of the "mvn test" command on the
appropriate module:
Running
### Commits
- [ ] My commits all reference appropriate Apache Helix GitHub issues in
their subject lines. In addition, my commits follow the guidelines from "[How
to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation (Optional)
- [ ] In case of new functionality, my PR adds documentation in the
following wiki page:
(Link the GitHub wiki you added)
### 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang opened a new issue #903: ConcurrentModificationException happens in the initHandler() logic
jiajunwang opened a new issue #903: ConcurrentModificationException happens in the initHandler() logic URL: https://github.com/apache/helix/issues/903 Example of the Exception. Note the version 0.9.1.507 is an internal minor version that has not been published publicly yet. But the problem exists in the older versions. And we need to fix it. java.util.ConcurrentModificationException: null at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:909) ~[?:1.8.0-zing_18.10.3.0] at java.util.ArrayList$Itr.next(ArrayList.java:859) ~[?:1.8.0-zing_18.10.3.0] at org.apache.helix.manager.zk.ZKHelixManager.initHandlers(ZKHelixManager.java:997) ~[helix-core-0.9.1.507.jar:0.9.1.507] at org.apache.helix.manager.zk.ZKHelixManager.handleNewSession(ZKHelixManager.java:1154) ~[helix-core-0.9.1.507.jar:0.9.1.507] at org.apache.helix.manager.zk.zookeeper.ZkClient$4.run(ZkClient.java:867) ~[helix-core-0.9.1.507.jar:0.9.1.507] at org.apache.helix.manager.zk.zookeeper.ZkEventThread.run(ZkEventThread.java:69) [helix-core-0.9.1.507.jar:0.9.1.507] 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on issue #851: Modify Helix generic controller to include new stage for customized view aggregation
zhangmeng916 commented on issue #851: Modify Helix generic controller to include new stage for customized view aggregation URL: https://github.com/apache/helix/pull/851#issuecomment-601939430 > One more thing, the added test cases are not enough. Please add tests for the following features: > > 1. We need to add a case in the test listener leakage for the new listener introduced. This one is tested in TestZkCallbackHandlerLeak. > 2. Admin change to create the new paths. This one is tested in TestZkHelixAdmin. > 3. In the integration test, please test adjusting the view config and see if the view changed accordingly. Moreover, we should use the customized view provider to test. Tested in TestComputeAndCleanupCustomizedView. The provider has unit test checked in, and the end to end integration test will cover from provider to router, which will be checked in by @mgao0. > 4. Try to cover some error case, for example, instances are not reporting the configured type. Or 2 instances are reporting different sets of types. Added test cases in TestComputeAndCleanupCustomizedView >Please try to cover all the use cases that we mentioned in the design doc. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #851: Modify Helix generic controller to include new stage for customized view aggregation
zhangmeng916 commented on a change in pull request #851: Modify Helix generic
controller to include new stage for customized view aggregation
URL: https://github.com/apache/helix/pull/851#discussion_r395884785
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##
@@ -569,12 +570,19 @@ public void
addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
});
}
+ @Override
+ public void
addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
+ String instanceName) throws Exception {
+addListener(listener, new
Builder(_clusterName).customizedStatesRoot(instanceName),
+ChangeType.CUSTOMIZED_STATE_ROOT, new
EventType[]{EventType.NodeChildrenChanged});
+ }
+
@Override
public void addCustomizedStateChangeListener(CustomizedStateChangeListener
listener,
- String instanceName, String customizedStateName) throws Exception {
-addListener(listener, new
Builder(_clusterName).customizedStates(instanceName, customizedStateName),
-ChangeType.CUSTOMIZED_STATE, new EventType[] {
EventType.NodeChildrenChanged
-});
+ String instanceName, String customizedStateType) throws Exception {
Review comment:
Yes, because under customized state, there're different types of customized
states, and their children are different resources, and then customized state
data. Only listen to one level of children is not enough.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
narendly commented on a change in pull request #899: Use Java Generics and
inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395846744
##
File path:
helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##
@@ -384,14 +373,26 @@ protected void validate() {
+ _zkAddress + " RealmAwareZkConnectionConfig: " +
_realmAwareZkConnectionConfig);
}
}
+ initializeConfigsIfNull();
+}
- // Resolve all default values
- if (_realmAwareZkConnectionConfig == null) {
-_realmAwareZkConnectionConfig =
-new
RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
- }
- if (_realmAwareZkClientConfig == null) {
-_realmAwareZkClientConfig = new
RealmAwareZkClient.RealmAwareZkClientConfig();
+/**
+ * Creates a RealmAwareZkClient for ZkHelixClusterVerifiers.
+ * Note that DedicatedZkClient is used whether it's multi-realm or
single-realm.
+ * @return
+ */
+private RealmAwareZkClient createZkClientFromBuilderForVerifier() {
Review comment:
@jiajunwang Because this would cause the constructor to take in a list of
parameters defined by its parent builder's createZkClient(). I'd argue that
it's not confusing because the method name makes is 100% clear - there really
is no ambiguity.
For ZkHelixClusterVerifier and its implementations, they, by definition,
only operate on single realm mode on a dedicated zk client. As such, that
degree of customization that would be required in the parent builder's
createZkClient() is not necessary.
Is it a good idea to have unnecessary parameters in a method just for the
sake of overriding a parent method, especially when it could be avoided?
Again, this is a stylistic choice, and for the sake of moving this forward,
I will make that change to make the method override its parent's method - that
way we would be on the same page.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang merged pull request #834: Complete the Routing Table Provider for CustomizedView
jiajunwang merged pull request #834: Complete the Routing Table Provider for CustomizedView URL: https://github.com/apache/helix/pull/834 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
jiajunwang commented on a change in pull request #899: Use Java Generics and
inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395812800
##
File path:
helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##
@@ -384,14 +373,26 @@ protected void validate() {
+ _zkAddress + " RealmAwareZkConnectionConfig: " +
_realmAwareZkConnectionConfig);
}
}
+ initializeConfigsIfNull();
+}
- // Resolve all default values
- if (_realmAwareZkConnectionConfig == null) {
-_realmAwareZkConnectionConfig =
-new
RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
- }
- if (_realmAwareZkClientConfig == null) {
-_realmAwareZkClientConfig = new
RealmAwareZkClient.RealmAwareZkClientConfig();
+/**
+ * Creates a RealmAwareZkClient for ZkHelixClusterVerifiers.
+ * Note that DedicatedZkClient is used whether it's multi-realm or
single-realm.
+ * @return
+ */
+private RealmAwareZkClient createZkClientFromBuilderForVerifier() {
Review comment:
This is not quite about the public interface. The ZkClient build behavior of
this builder should be this logic, right? So we should override the method
here. Unless we do have a use case of this specific builder that requires to
call the parent class createZkClient(). Otherwise, it just causes confusing IMO.
What is your concern not doing this?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on issue #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
narendly commented on issue #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders URL: https://github.com/apache/helix/pull/899#issuecomment-601842437 > > > Looks good in general. > > > The only thing concerns me is the newly modified deprecated constructors. We should either keep the same behavior as the master branch and then mark them as deprecated, or change them to use the new builder logic completely. The current diff on the zooscalability branch is mixed. > > > Deprecating them would be easier and safer. But we don't need to touch them in this PR. > > > So can we have a small follow-up PR to: > > > > > > 1. revert the newly modified constructors that are not onboarding to the new builder. Since builder should be the new way we create an accessor/verifier that uses a realm-aware ZkClient, right? > > > 2. mark the old constructor deprecated. > > > > > > This has been discussed offline among other PMC and committers, and we have considered this possibility, but we decided that in order to make rollout and adoption smoother, we have opted for this design. This way, users can easily test out the behavior without having to make any code change. Does that make sense to you? > > The behavior in fact is exactly same. The difference kicks in only if the user sets the multiZk System config. Also note that the end-user who uses Helix Java APIs do not know what kind of zkclient is returned - it is an internal detail that belongs inside Java API construction logic. > > As far as making a new method deprecated, I think there's a tradeoff here. I do not have to deprecate it if you're not comfortable with it, but the method itself is needed in order to promote code reuse. It's just the logic that should be deprecated. > > I discussed with Hunter offline, design-wise, I'm totally fine. But implementation-wise, we do have 2 options here. 1. Creating a builder might be overkill because it is for backward compatibility only. 2. On the other hand, my concern for the current design is that, if we have multiple zkclient user classes have such kind usage, we will need to do if-else check based on the system property MULTI_ZK_ENABLED multiple times. And that property is also for backward compatibility only, which will be eventually removed. There is a trade-off we need to consider. > But, as I commented before, there is no need to be blocked on this topic in this PR. Thanks for summarizing our discussion. In terms of your last point, let me also add that I am not a big fan of the idea of creating a Builder just to make things backward-compatible because 1) it doesn't reduce duplicate code (in fact, it may end up adding more boilerplate code) 2) there is no clear common logic other than reading from System.Properties, and 3) I'd argue that sometimes a simple if-else does the trick and is the easiest to understand. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
narendly commented on a change in pull request #899: Use Java Generics and
inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395809363
##
File path:
helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##
@@ -384,14 +373,26 @@ protected void validate() {
+ _zkAddress + " RealmAwareZkConnectionConfig: " +
_realmAwareZkConnectionConfig);
}
}
+ initializeConfigsIfNull();
+}
- // Resolve all default values
- if (_realmAwareZkConnectionConfig == null) {
-_realmAwareZkConnectionConfig =
-new
RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
- }
- if (_realmAwareZkClientConfig == null) {
-_realmAwareZkClientConfig = new
RealmAwareZkClient.RealmAwareZkClientConfig();
+/**
+ * Creates a RealmAwareZkClient for ZkHelixClusterVerifiers.
+ * Note that DedicatedZkClient is used whether it's multi-realm or
single-realm.
+ * @return
+ */
+private RealmAwareZkClient createZkClientFromBuilderForVerifier() {
Review comment:
Turns out we don't need to override that here. This createZkClient() is
private and has different logic for verifiers. Just to keep it clear that this
logic is different from the regular createZkClient, I will keep the method name
as is. Again, I'd like to emphasize that there's no extra public method so we
are not adding anything to the interface.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders URL: https://github.com/apache/helix/pull/899#discussion_r395807333 ## File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java ## @@ -126,41 +117,9 @@ public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String chrootPath, Review comment: This is already deprecated.. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on issue #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
jiajunwang commented on issue #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders URL: https://github.com/apache/helix/pull/899#issuecomment-601838962 > > Looks good in general. > > The only thing concerns me is the newly modified deprecated constructors. We should either keep the same behavior as the master branch and then mark them as deprecated, or change them to use the new builder logic completely. The current diff on the zooscalability branch is mixed. > > Deprecating them would be easier and safer. But we don't need to touch them in this PR. > > So can we have a small follow-up PR to: > > > > 1. revert the newly modified constructors that are not onboarding to the new builder. Since builder should be the new way we create an accessor/verifier that uses a realm-aware ZkClient, right? > > 2. mark the old constructor deprecated. > > This has been discussed offline among other PMC and committers, and we have considered this possibility, but we decided that in order to make rollout and adoption smoother, we have opted for this design. This way, users can easily test out the behavior without having to make any code change. Does that make sense to you? > > The behavior in fact is exactly same. The difference kicks in only if the user sets the multiZk System config. Also note that the end-user who uses Helix Java APIs do not know what kind of zkclient is returned - it is an internal detail that belongs inside Java API construction logic. > > As far as making a new method deprecated, I think there's a tradeoff here. I do not have to deprecate it if you're not comfortable with it, but the method itself is needed in order to promote code reuse. It's just the logic that should be deprecated. I discussed with Hunter offline, design-wise, I'm totally fine. But implementation-wise, we do have 2 options here. 1. Creating a builder might be overkill because it is for backward compatibility only. 2. On the other hand, my concern for the current design is that, if we have multiple zkclient user classes have such kind usage, we will need to do if-else check based on the system property MULTI_ZK_ENABLED multiple times. And that property is also for backward compatibility only, which will be eventually removed. There is a trade-off we need to consider. But, as I commented before, there is no need to be blocked on this topic in this PR. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
narendly commented on a change in pull request #899: Use Java Generics and
inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395806370
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##
@@ -1390,65 +1311,23 @@ public Builder() {
validate();
return new ZkBaseDataAccessor<>(this);
}
-
-/*
- * Validates the given parameters before building an instance of
ZkBaseDataAccessor.
- */
-private void validate() {
- // Resolve RealmMode based on other parameters
- boolean isZkAddressSet = zkAddress != null && !zkAddress.isEmpty();
- boolean isZkClientTypeSet = zkClientType != null;
-
- // If ZkClientType is set, RealmMode must either be single-realm or not
set.
- if (isZkClientTypeSet && realmMode ==
RealmAwareZkClient.RealmMode.MULTI_REALM) {
-throw new HelixException("ZkClientType cannot be set on multi-realm
mode!");
- }
- // If ZkClientType is not set and realmMode is single-realm, default to
SHARED
- if (!isZkClientTypeSet && realmMode ==
RealmAwareZkClient.RealmMode.SINGLE_REALM) {
-zkClientType = ZkClientType.SHARED;
- }
-
- if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM &&
!isZkAddressSet) {
-throw new HelixException("RealmMode cannot be single-realm without a
valid ZkAddress set!");
- }
-
- if (realmMode == RealmAwareZkClient.RealmMode.MULTI_REALM &&
isZkAddressSet) {
-throw new HelixException("ZkAddress cannot be set on multi-realm
mode!");
- }
-
- if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM
- && zkClientType == ZkClientType.FEDERATED) {
-throw new HelixException("FederatedZkClient cannot be set on
single-realm mode!");
- }
-
- if (realmMode == null) {
-realmMode = isZkAddressSet ? RealmAwareZkClient.RealmMode.SINGLE_REALM
-: RealmAwareZkClient.RealmMode.MULTI_REALM;
- }
-
- // Resolve RealmAwareZkClientConfig
- if (realmAwareZkClientConfig == null) {
-realmAwareZkClientConfig = new
RealmAwareZkClient.RealmAwareZkClientConfig()
-.setZkSerializer(new ZNRecordSerializer());
- }
-
- // Resolve RealmAwareZkConnectionConfig
- if (realmAwareZkConnectionConfig == null) {
-// If not set, create a default one
-realmAwareZkConnectionConfig =
-new
RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
- }
-}
}
- /*
- * This is used for constructors that do not take a Builder in as a
parameter because of
- * keeping backward-compatibility.
+ /**
+ * This method is used for constructors that are not based on the Builder for
+ * backward-compatibility.
+ * It checks if there is a System Property config set for Multi-ZK mode and
determines if a
+ * FederatedZkClient should be created.
+ * @param clientConfig default RealmAwareZkClientConfig with ZK serializer
set
+ * @param zkAddress
+ * @param zkClientType
+ * @return
*/
- private RealmAwareZkClient buildRealmAwareZkClient(
+ static RealmAwareZkClient buildRealmAwareZkClientWithDefaultConfigs(
Review comment:
Discussed offline.
As for deprecating the new little method stub, I will remove the deprecated
annotation, but i think we need to look at it as an improvement we are making
to the existing code because this is **not** newly added logic. we have
refactored it to cut down on duplicate code. The method stub is created solely
for that purpose.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders URL: https://github.com/apache/helix/pull/899#discussion_r395734888 ## File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java ## @@ -126,41 +117,9 @@ public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String chrootPath, Review comment: I will deprecate this but I do not agree with that we should revert the behavior. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
narendly commented on a change in pull request #899: Use Java Generics and
inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395734207
##
File path:
helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##
@@ -384,14 +373,26 @@ protected void validate() {
+ _zkAddress + " RealmAwareZkConnectionConfig: " +
_realmAwareZkConnectionConfig);
}
}
+ initializeConfigsIfNull();
+}
- // Resolve all default values
- if (_realmAwareZkConnectionConfig == null) {
-_realmAwareZkConnectionConfig =
-new
RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
- }
- if (_realmAwareZkClientConfig == null) {
-_realmAwareZkClientConfig = new
RealmAwareZkClient.RealmAwareZkClientConfig();
+/**
+ * Creates a RealmAwareZkClient for ZkHelixClusterVerifiers.
+ * Note that DedicatedZkClient is used whether it's multi-realm or
single-realm.
+ * @return
+ */
+private RealmAwareZkClient createZkClientFromBuilderForVerifier() {
Review comment:
We can do that.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] alirezazamani commented on issue #834: Complete the Routing Table Provider for CustomizedView
alirezazamani commented on issue #834: Complete the Routing Table Provider for CustomizedView URL: https://github.com/apache/helix/pull/834#issuecomment-601773757 This PR is ready to be merged, approved by @zhangmeng916 Final commit message: Complete the Routing Table Provider for CustomizedView In this commit, the routing table provider has been changed in a way to include customized view feature. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
jiajunwang commented on a change in pull request #899: Use Java Generics and
inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395454217
##
File path:
helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##
@@ -384,14 +373,26 @@ protected void validate() {
+ _zkAddress + " RealmAwareZkConnectionConfig: " +
_realmAwareZkConnectionConfig);
}
}
+ initializeConfigsIfNull();
+}
- // Resolve all default values
- if (_realmAwareZkConnectionConfig == null) {
-_realmAwareZkConnectionConfig =
-new
RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
- }
- if (_realmAwareZkClientConfig == null) {
-_realmAwareZkClientConfig = new
RealmAwareZkClient.RealmAwareZkClientConfig();
+/**
+ * Creates a RealmAwareZkClient for ZkHelixClusterVerifiers.
+ * Note that DedicatedZkClient is used whether it's multi-realm or
single-realm.
+ * @return
+ */
+private RealmAwareZkClient createZkClientFromBuilderForVerifier() {
Review comment:
Why not override createZkClient() here?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders URL: https://github.com/apache/helix/pull/899#discussion_r395453530 ## File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java ## @@ -126,41 +117,9 @@ public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String chrootPath, Review comment: This one shall be deprecated as well. And I would suggest keeping the very original behavior. Only return realmaware client through the new builder method. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
jiajunwang commented on a change in pull request #899: Use Java Generics and
inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395452606
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##
@@ -1390,65 +1311,23 @@ public Builder() {
validate();
return new ZkBaseDataAccessor<>(this);
}
-
-/*
- * Validates the given parameters before building an instance of
ZkBaseDataAccessor.
- */
-private void validate() {
- // Resolve RealmMode based on other parameters
- boolean isZkAddressSet = zkAddress != null && !zkAddress.isEmpty();
- boolean isZkClientTypeSet = zkClientType != null;
-
- // If ZkClientType is set, RealmMode must either be single-realm or not
set.
- if (isZkClientTypeSet && realmMode ==
RealmAwareZkClient.RealmMode.MULTI_REALM) {
-throw new HelixException("ZkClientType cannot be set on multi-realm
mode!");
- }
- // If ZkClientType is not set and realmMode is single-realm, default to
SHARED
- if (!isZkClientTypeSet && realmMode ==
RealmAwareZkClient.RealmMode.SINGLE_REALM) {
-zkClientType = ZkClientType.SHARED;
- }
-
- if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM &&
!isZkAddressSet) {
-throw new HelixException("RealmMode cannot be single-realm without a
valid ZkAddress set!");
- }
-
- if (realmMode == RealmAwareZkClient.RealmMode.MULTI_REALM &&
isZkAddressSet) {
-throw new HelixException("ZkAddress cannot be set on multi-realm
mode!");
- }
-
- if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM
- && zkClientType == ZkClientType.FEDERATED) {
-throw new HelixException("FederatedZkClient cannot be set on
single-realm mode!");
- }
-
- if (realmMode == null) {
-realmMode = isZkAddressSet ? RealmAwareZkClient.RealmMode.SINGLE_REALM
-: RealmAwareZkClient.RealmMode.MULTI_REALM;
- }
-
- // Resolve RealmAwareZkClientConfig
- if (realmAwareZkClientConfig == null) {
-realmAwareZkClientConfig = new
RealmAwareZkClient.RealmAwareZkClientConfig()
-.setZkSerializer(new ZNRecordSerializer());
- }
-
- // Resolve RealmAwareZkConnectionConfig
- if (realmAwareZkConnectionConfig == null) {
-// If not set, create a default one
-realmAwareZkConnectionConfig =
-new
RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
- }
-}
}
- /*
- * This is used for constructors that do not take a Builder in as a
parameter because of
- * keeping backward-compatibility.
+ /**
+ * This method is used for constructors that are not based on the Builder for
+ * backward-compatibility.
+ * It checks if there is a System Property config set for Multi-ZK mode and
determines if a
+ * FederatedZkClient should be created.
+ * @param clientConfig default RealmAwareZkClientConfig with ZK serializer
set
+ * @param zkAddress
+ * @param zkClientType
+ * @return
*/
- private RealmAwareZkClient buildRealmAwareZkClient(
+ static RealmAwareZkClient buildRealmAwareZkClientWithDefaultConfigs(
Review comment:
There is no need to deprecate, we can directly remove it. I checked, it is
not in the master branch yet.
Same for the other usages. I think we should just remove it instead of
introducing a new but deprecated method to the master branch.
If you are worried about the backward compatibility, then we can just leave
the very old code that creates either DEDICATED or SHARED client. Those
constructors are deprecated anyway.
We don't need to change the logic of the deprecated methods to return
RealmAware ZkClient. As you said, this will just mix the new logic with the
old/deprecated ones.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
jiajunwang commented on a change in pull request #899: Use Java Generics and
inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395450703
##
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/GenericZkHelixApiBuilder.java
##
@@ -0,0 +1,142 @@
+package org.apache.helix.manager.zk;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.io.IOException;
+
+import org.apache.helix.HelixException;
+import org.apache.helix.msdcommon.exception.InvalidRoutingDataException;
+import org.apache.helix.zookeeper.api.client.HelixZkClient;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer;
+import org.apache.helix.zookeeper.impl.client.FederatedZkClient;
+import org.apache.helix.zookeeper.impl.factory.SharedZkClientFactory;
+
+
+/**
+ * GenericZkHelixApiBuilder serves as the abstract parent class for Builders
used by Helix Java APIs
+ * that create ZK connections. By having this class, we reduce duplicate code
as much as possible.
+ * @param
+ */
+public abstract class GenericZkHelixApiBuilder> {
Review comment:
Just to put some details of what we discussed, the only benefit of adding
the build() method to the parent builder class is more complete interface
design. But as Hunter mentioned, it is optional. I don't have a strong
preference on this change, we will have the build() method for all the
non-abstract builders anyway.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
