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 2020/03/05 02:11:07 UTC
[GitHub] [helix] narendly opened a new pull request #861: Make ClusterSetup
realm-aware
narendly opened a new pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
#860
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
We make ClusterSetup, a Helix Java API, realm-aware so that this could be used in a multi-ZK environment.
Changelist:
1. Add a Builder to enable users to set internal ZkClient parameters
2. Add the realm-aware behavior in existing constructors
### Tests
- [x] The following tests are written for this issue:
TestClusterSetup covers this.
- [ ] The following is the result of the "mvn test" command on the appropriate module:
(Copy & paste the result of "mvn test")
### Commits
- [x] 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
- [x] 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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r389157327
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -135,37 +138,74 @@
public static final String removeConstraint = "removeConstraint";
private static final Logger _logger = LoggerFactory.getLogger(ClusterSetup.class);
- private final String _zkServerAddress;
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ private String _zkServerAddress;
+ private final RealmAwareZkClient _zkClient;
+ // true if ZkBaseDataAccessor was instantiated with a RealmAwareZkClient, false otherwise
// This is used for close() to determine how ZkBaseDataAccessor should close the underlying
// ZkClient
private final boolean _usesExternalZkClient;
private final HelixAdmin _admin;
public ClusterSetup(String zkServerAddress) {
_zkServerAddress = zkServerAddress;
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(_zkServerAddress));
- _zkClient.setZkSerializer(new ZNRecordSerializer());
+
+ // First, try to start on multi-realm mode using FederatedZkClient
+ RealmAwareZkClient zkClient;
+ try {
+ zkClient = new FederatedZkClient(
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(),
+ new RealmAwareZkClient.RealmAwareZkClientConfig());
+ } catch (InvalidRoutingDataException | IOException | IllegalStateException e) {
+ // Note: IllegalStateException is for HttpRoutingDataReader if MSDS endpoint cannot be found
+ // Fall back to single-realm mode using SharedZkClient (HelixZkClient)
+ // This is to preserve backward-compatibility
+ if (zkServerAddress == null || zkServerAddress.isEmpty()) {
+ throw new IllegalArgumentException("ZK server address is null or empty!");
Review comment:
If we want to preserve the existing behavior, I say we just remove this check (because the original implementation doesn't have this check).
I am not sure what you're asking exactly... could you explain?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r389148987
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -1570,4 +1610,66 @@ public static void main(String[] args) throws Exception {
int ret = processCommandLineArgs(args);
System.exit(ret);
}
+
+ public static class Builder {
Review comment:
There aren't any documents. However, I think this input is valid.
Let's use this to track this issue. I think it would be wise to first focus on making Java APIs realm-aware, and take a step back and look at all of our Helix Java API classes and determine how we should refactor the Builders.
https://github.com/apache/helix/issues/873
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r389152966
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -135,37 +138,74 @@
public static final String removeConstraint = "removeConstraint";
private static final Logger _logger = LoggerFactory.getLogger(ClusterSetup.class);
- private final String _zkServerAddress;
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ private String _zkServerAddress;
Review comment:
If the user wants to create this class on multi-realm mode, FederatedZkClient will be used. FederatedZkClient does not and should not expose ZkAddress because it's an internal state info.
In fact, this `_zkServerAddress` was not being used anywhere and doesn't need to exist. I've removed this field from this class.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r389074928
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -1570,4 +1610,66 @@ public static void main(String[] args) throws Exception {
int ret = processCommandLineArgs(args);
System.exit(ret);
}
+
+ public static class Builder {
Review comment:
For 2) the setter and validation logic should be common for the ZK parameters. And ZK related input and validation will be evolved the same. That's why a single class would help us to reduce maintenance costs.
For 1) There are multiple options to avoid the problem. Not sure if we have these options evaluated. If any document, that would be great so I can take a look : )
1. Pass the ZK Client builder to the verifier's builders. So there is no inheritance, but the ZK client building logic is in the client builder only.
2. Use a template class as mentioned here, https://stackoverflow.com/questions/4031857/way-to-make-java-parent-class-method-return-object-of-child-class
3. Since ZK parameters are in most cases requried, include them in the constructor and you don't need to worry about the return value. In the verifier builder, the constructor calls the super(...) constructor to finish the ZK client build.
There could be more ways.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r389148987
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -1570,4 +1610,66 @@ public static void main(String[] args) throws Exception {
int ret = processCommandLineArgs(args);
System.exit(ret);
}
+
+ public static class Builder {
Review comment:
There aren't any documents. However, I think this input is valid.
Let's use [this](https://github.com/apache/helix/issues/873) to track this issue. I think it would be wise to first focus on making Java APIs realm-aware, and take a step back and look at all of our Helix Java API classes and determine how we should refactor the Builders.
https://github.com/apache/helix/issues/873
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r388728364
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -78,6 +78,7 @@
import org.apache.helix.util.HelixUtil;
import org.apache.helix.util.RebalanceUtil;
import org.apache.helix.zookeeper.api.client.HelixZkClient;
Review comment:
nit, is this import needed anymore?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r388709568
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -135,37 +138,74 @@
public static final String removeConstraint = "removeConstraint";
private static final Logger _logger = LoggerFactory.getLogger(ClusterSetup.class);
- private final String _zkServerAddress;
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ private String _zkServerAddress;
+ private final RealmAwareZkClient _zkClient;
+ // true if ZkBaseDataAccessor was instantiated with a RealmAwareZkClient, false otherwise
// This is used for close() to determine how ZkBaseDataAccessor should close the underlying
// ZkClient
private final boolean _usesExternalZkClient;
private final HelixAdmin _admin;
public ClusterSetup(String zkServerAddress) {
_zkServerAddress = zkServerAddress;
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(_zkServerAddress));
- _zkClient.setZkSerializer(new ZNRecordSerializer());
+
+ // First, try to start on multi-realm mode using FederatedZkClient
+ RealmAwareZkClient zkClient;
+ try {
+ zkClient = new FederatedZkClient(
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(),
+ new RealmAwareZkClient.RealmAwareZkClientConfig());
+ } catch (InvalidRoutingDataException | IOException | IllegalStateException e) {
+ // Note: IllegalStateException is for HttpRoutingDataReader if MSDS endpoint cannot be found
+ // Fall back to single-realm mode using SharedZkClient (HelixZkClient)
+ // This is to preserve backward-compatibility
+ if (zkServerAddress == null || zkServerAddress.isEmpty()) {
+ throw new IllegalArgumentException("ZK server address is null or empty!");
+ }
+ zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(_zkServerAddress));
+ zkClient.setZkSerializer(new ZNRecordSerializer());
+ }
+
+ _zkClient = zkClient;
_admin = new ZKHelixAdmin(_zkClient);
_usesExternalZkClient = false;
}
- public ClusterSetup(HelixZkClient zkClient) {
+ public ClusterSetup(RealmAwareZkClient zkClient) {
_zkServerAddress = zkClient.getServers();
_zkClient = zkClient;
_admin = new ZKHelixAdmin(_zkClient);
_usesExternalZkClient = true;
}
- public ClusterSetup(HelixZkClient zkClient, HelixAdmin zkHelixAdmin) {
+ public ClusterSetup(RealmAwareZkClient zkClient, HelixAdmin zkHelixAdmin) {
_zkServerAddress = zkClient.getServers();
_zkClient = zkClient;
_admin = zkHelixAdmin;
_usesExternalZkClient = true;
}
+ private ClusterSetup(Builder builder) throws IOException, InvalidRoutingDataException {
Review comment:
Like you mentioned, do we need the checked exception signature? If yes, what is the consideration to have it thrown checked exceptions?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r389042321
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -135,37 +138,74 @@
public static final String removeConstraint = "removeConstraint";
private static final Logger _logger = LoggerFactory.getLogger(ClusterSetup.class);
- private final String _zkServerAddress;
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ private String _zkServerAddress;
+ private final RealmAwareZkClient _zkClient;
+ // true if ZkBaseDataAccessor was instantiated with a RealmAwareZkClient, false otherwise
// This is used for close() to determine how ZkBaseDataAccessor should close the underlying
// ZkClient
private final boolean _usesExternalZkClient;
private final HelixAdmin _admin;
public ClusterSetup(String zkServerAddress) {
_zkServerAddress = zkServerAddress;
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(_zkServerAddress));
- _zkClient.setZkSerializer(new ZNRecordSerializer());
+
+ // First, try to start on multi-realm mode using FederatedZkClient
+ RealmAwareZkClient zkClient;
+ try {
+ zkClient = new FederatedZkClient(
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(),
+ new RealmAwareZkClient.RealmAwareZkClientConfig());
+ } catch (InvalidRoutingDataException | IOException | IllegalStateException e) {
+ // Note: IllegalStateException is for HttpRoutingDataReader if MSDS endpoint cannot be found
+ // Fall back to single-realm mode using SharedZkClient (HelixZkClient)
+ // This is to preserve backward-compatibility
+ if (zkServerAddress == null || zkServerAddress.isEmpty()) {
+ throw new IllegalArgumentException("ZK server address is null or empty!");
+ }
+ zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(_zkServerAddress));
+ zkClient.setZkSerializer(new ZNRecordSerializer());
+ }
+
+ _zkClient = zkClient;
_admin = new ZKHelixAdmin(_zkClient);
_usesExternalZkClient = false;
}
- public ClusterSetup(HelixZkClient zkClient) {
+ public ClusterSetup(RealmAwareZkClient zkClient) {
_zkServerAddress = zkClient.getServers();
_zkClient = zkClient;
_admin = new ZKHelixAdmin(_zkClient);
_usesExternalZkClient = true;
}
- public ClusterSetup(HelixZkClient zkClient, HelixAdmin zkHelixAdmin) {
+ public ClusterSetup(RealmAwareZkClient zkClient, HelixAdmin zkHelixAdmin) {
_zkServerAddress = zkClient.getServers();
_zkClient = zkClient;
_admin = zkHelixAdmin;
_usesExternalZkClient = true;
}
+ private ClusterSetup(Builder builder) throws IOException, InvalidRoutingDataException {
Review comment:
Good cross validation. This shouldn't be needed. Let me update the PR :+1:
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r389044068
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -1570,4 +1610,66 @@ public static void main(String[] args) throws Exception {
int ret = processCommandLineArgs(args);
System.exit(ret);
}
+
+ public static class Builder {
Review comment:
@jiajunwang This was an option considered carefully, but let me note here that having a parent builder doesn't actually remove duplicate code because all setters and constructors will have to either be re-written or overriden. This is because setters return Builder objects that aren't compatible with children builders. Take a look at the ZkClusterVerifier PR - in that PR, it makes more sense to have a parent Builder, but we don't see the reduction in duplicate code.
In general, having an inheritance hierarchy for static Builders is not necessarily a good idea because 1) it doesn't reduce code duplication, 2) Builder's setter and validation logic need to / may evolve differently, and inheritance might make that difficult in the future.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r389152966
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -135,37 +138,74 @@
public static final String removeConstraint = "removeConstraint";
private static final Logger _logger = LoggerFactory.getLogger(ClusterSetup.class);
- private final String _zkServerAddress;
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ private String _zkServerAddress;
Review comment:
If the user wants to create this class on multi-realm mode, FederatedZkClient will be used. FederatedZkClient does not and should not expose ZkAddress because it's an internal state info.
In fact, this `_zkServerAddress` was not being used anywhere and doesn't need to exist. I've removed this field from this class for further safety. I think we want to avoid exposing specific ZK server-related details as much as possible in a multi-zk environment to prevent misuse and unauthorized operations.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r389044068
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -1570,4 +1610,66 @@ public static void main(String[] args) throws Exception {
int ret = processCommandLineArgs(args);
System.exit(ret);
}
+
+ public static class Builder {
Review comment:
@jiajunwang This was an option considered, but let me note here that having a parent builder doesn't actually remove duplicate code because all setters and constructors will have to either be re-written or overriden. This is because setters return Builder objects that aren't compatible with children builders. Take a look at the ZkClusterVerifier PR - in that PR, it makes more sense to have a parent Builder, but we don't see the reduction in duplicate code.
In general, having an inheritance hierarchy for static Builders is not necessarily a good idea because 1) it doesn't reduce code duplication, 2) Builder's setter and validation logic need to / may evolve differently, and inheritance might make that difficult in the future.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly merged pull request #861: Make ClusterSetup
realm-aware
Posted by GitBox <gi...@apache.org>.
narendly merged pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r389157723
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -135,37 +138,74 @@
public static final String removeConstraint = "removeConstraint";
private static final Logger _logger = LoggerFactory.getLogger(ClusterSetup.class);
- private final String _zkServerAddress;
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ private String _zkServerAddress;
+ private final RealmAwareZkClient _zkClient;
+ // true if ZkBaseDataAccessor was instantiated with a RealmAwareZkClient, false otherwise
// This is used for close() to determine how ZkBaseDataAccessor should close the underlying
// ZkClient
private final boolean _usesExternalZkClient;
private final HelixAdmin _admin;
public ClusterSetup(String zkServerAddress) {
_zkServerAddress = zkServerAddress;
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(_zkServerAddress));
- _zkClient.setZkSerializer(new ZNRecordSerializer());
+
+ // First, try to start on multi-realm mode using FederatedZkClient
+ RealmAwareZkClient zkClient;
+ try {
+ zkClient = new FederatedZkClient(
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(),
+ new RealmAwareZkClient.RealmAwareZkClientConfig());
+ } catch (InvalidRoutingDataException | IOException | IllegalStateException e) {
+ // Note: IllegalStateException is for HttpRoutingDataReader if MSDS endpoint cannot be found
+ // Fall back to single-realm mode using SharedZkClient (HelixZkClient)
+ // This is to preserve backward-compatibility
+ if (zkServerAddress == null || zkServerAddress.isEmpty()) {
+ throw new IllegalArgumentException("ZK server address is null or empty!");
+ }
+ zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(_zkServerAddress));
+ zkClient.setZkSerializer(new ZNRecordSerializer());
+ }
+
+ _zkClient = zkClient;
_admin = new ZKHelixAdmin(_zkClient);
_usesExternalZkClient = false;
}
- public ClusterSetup(HelixZkClient zkClient) {
+ public ClusterSetup(RealmAwareZkClient zkClient) {
_zkServerAddress = zkClient.getServers();
_zkClient = zkClient;
_admin = new ZKHelixAdmin(_zkClient);
_usesExternalZkClient = true;
}
- public ClusterSetup(HelixZkClient zkClient, HelixAdmin zkHelixAdmin) {
+ public ClusterSetup(RealmAwareZkClient zkClient, HelixAdmin zkHelixAdmin) {
_zkServerAddress = zkClient.getServers();
_zkClient = zkClient;
_admin = zkHelixAdmin;
_usesExternalZkClient = true;
}
+ private ClusterSetup(Builder builder) throws IOException, InvalidRoutingDataException {
Review comment:
Discussed with @pkuwm offline.
We decided to keep these checked exceptions. As a matter of fact, all builder-based constructors should throw checked exceptions because things could go wrong on multi-realm mode.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r391343848
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -123,23 +127,23 @@ public ConfigAccessor(RealmAwareZkClient zkClient) {
* @param zkAddress
*/
public ConfigAccessor(String zkAddress) {
- // First, attempt to connect on multi-realm mode using FederatedZkClient
- RealmAwareZkClient zkClient;
- try {
- zkClient = new FederatedZkClient(
- new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(),
- new RealmAwareZkClient.RealmAwareZkClientConfig());
- } catch (IOException | InvalidRoutingDataException | IllegalStateException e) {
- // Connecting multi-realm failed - fall back to creating it on single-realm mode using the given ZK address
- LOG.info(
- "ConfigAccessor: not able to connect on multi-realm mode; connecting single-realm mode to ZK: {}",
- zkAddress, e);
- zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
- new HelixZkClient.ZkClientConfig().setZkSerializer(new ZNRecordSerializer()));
- }
- _zkClient = zkClient;
_usesExternalZkClient = false;
+
+ // If the multi ZK config is enabled, use FederatedZkClient on multi-realm mode
+ if (Boolean.parseBoolean(System.getProperty(SystemPropertyKeys.MULTI_ZK_ENABLED))) {
Review comment:
They should both be considered as true. Either way works.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r388712466
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -135,37 +138,74 @@
public static final String removeConstraint = "removeConstraint";
private static final Logger _logger = LoggerFactory.getLogger(ClusterSetup.class);
- private final String _zkServerAddress;
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ private String _zkServerAddress;
+ private final RealmAwareZkClient _zkClient;
+ // true if ZkBaseDataAccessor was instantiated with a RealmAwareZkClient, false otherwise
// This is used for close() to determine how ZkBaseDataAccessor should close the underlying
// ZkClient
private final boolean _usesExternalZkClient;
private final HelixAdmin _admin;
public ClusterSetup(String zkServerAddress) {
_zkServerAddress = zkServerAddress;
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(_zkServerAddress));
- _zkClient.setZkSerializer(new ZNRecordSerializer());
+
+ // First, try to start on multi-realm mode using FederatedZkClient
+ RealmAwareZkClient zkClient;
+ try {
+ zkClient = new FederatedZkClient(
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(),
+ new RealmAwareZkClient.RealmAwareZkClientConfig());
+ } catch (InvalidRoutingDataException | IOException | IllegalStateException e) {
+ // Note: IllegalStateException is for HttpRoutingDataReader if MSDS endpoint cannot be found
+ // Fall back to single-realm mode using SharedZkClient (HelixZkClient)
+ // This is to preserve backward-compatibility
+ if (zkServerAddress == null || zkServerAddress.isEmpty()) {
+ throw new IllegalArgumentException("ZK server address is null or empty!");
Review comment:
If zk address is not set for multi-realm mode, a user would not expect an `IllegalArgumentException`. Shall we throw an `IllegalStateException` as the user doesn't set this **argument**? And also include the original exception message/stack?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r391125269
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -123,23 +127,23 @@ public ConfigAccessor(RealmAwareZkClient zkClient) {
* @param zkAddress
*/
public ConfigAccessor(String zkAddress) {
- // First, attempt to connect on multi-realm mode using FederatedZkClient
- RealmAwareZkClient zkClient;
- try {
- zkClient = new FederatedZkClient(
- new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(),
- new RealmAwareZkClient.RealmAwareZkClientConfig());
- } catch (IOException | InvalidRoutingDataException | IllegalStateException e) {
- // Connecting multi-realm failed - fall back to creating it on single-realm mode using the given ZK address
- LOG.info(
- "ConfigAccessor: not able to connect on multi-realm mode; connecting single-realm mode to ZK: {}",
- zkAddress, e);
- zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
- new HelixZkClient.ZkClientConfig().setZkSerializer(new ZNRecordSerializer()));
- }
- _zkClient = zkClient;
_usesExternalZkClient = false;
+
+ // If the multi ZK config is enabled, use FederatedZkClient on multi-realm mode
+ if (Boolean.parseBoolean(System.getProperty(SystemPropertyKeys.MULTI_ZK_ENABLED))) {
Review comment:
Nit: not sure whether or not we would allow users to set upper case “TRUE” and honor it as true. I just use getBoolean() to restrict it to true if and only if string is lowercase “true”.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r389044068
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -1570,4 +1610,66 @@ public static void main(String[] args) throws Exception {
int ret = processCommandLineArgs(args);
System.exit(ret);
}
+
+ public static class Builder {
Review comment:
@jiajunwang This was an option considered carefully, but let me note here that having a parent builder doesn't actually remove duplicate code because all setters and constructors will have to either be re-written or overriden. This is because setters return Builder objects that aren't compatible with children builders. Take a look at the ZkClusterVerifier PR - in that PR, it makes more sense to have a parent Builder, but we don't see the reduction in duplicate code.
In general, having an inheritance hierarchy for static Builders is not necessarily a good idea because 1) it doesn't reduce code duplication, 2) Builder's setter and validation logic need to / may evolve differently, and inheritance might make that difficult in the future.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r388728606
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -135,37 +138,74 @@
public static final String removeConstraint = "removeConstraint";
private static final Logger _logger = LoggerFactory.getLogger(ClusterSetup.class);
- private final String _zkServerAddress;
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ private String _zkServerAddress;
Review comment:
It seems this field can still be final. Why do we need to remove the final keyword?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r389152966
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -135,37 +138,74 @@
public static final String removeConstraint = "removeConstraint";
private static final Logger _logger = LoggerFactory.getLogger(ClusterSetup.class);
- private final String _zkServerAddress;
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ private String _zkServerAddress;
Review comment:
If the user wants to create this class on multi-realm mode, FederatedZkClient will be used. FederatedZkClient does not and should not expose ZkAddress because it's an internal state info.
In fact, this `_zkServerAddress` was not being used anywhere and doesn't need to exist. I've removed this field from this class for further safety.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r388729278
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -1570,4 +1610,66 @@ public static void main(String[] args) throws Exception {
int ret = processCommandLineArgs(args);
System.exit(ret);
}
+
+ public static class Builder {
Review comment:
I see similar builders in multiple PRs for the different accessor or tools. Can we have a parent ZkClient builder which contains basic info and validation, and these builders can be the children of that parent.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r388710079
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -135,37 +138,74 @@
public static final String removeConstraint = "removeConstraint";
private static final Logger _logger = LoggerFactory.getLogger(ClusterSetup.class);
- private final String _zkServerAddress;
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ private String _zkServerAddress;
+ private final RealmAwareZkClient _zkClient;
+ // true if ZkBaseDataAccessor was instantiated with a RealmAwareZkClient, false otherwise
// This is used for close() to determine how ZkBaseDataAccessor should close the underlying
// ZkClient
private final boolean _usesExternalZkClient;
private final HelixAdmin _admin;
public ClusterSetup(String zkServerAddress) {
_zkServerAddress = zkServerAddress;
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(_zkServerAddress));
- _zkClient.setZkSerializer(new ZNRecordSerializer());
+
+ // First, try to start on multi-realm mode using FederatedZkClient
+ RealmAwareZkClient zkClient;
+ try {
+ zkClient = new FederatedZkClient(
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(),
+ new RealmAwareZkClient.RealmAwareZkClientConfig());
+ } catch (InvalidRoutingDataException | IOException | IllegalStateException e) {
+ // Note: IllegalStateException is for HttpRoutingDataReader if MSDS endpoint cannot be found
+ // Fall back to single-realm mode using SharedZkClient (HelixZkClient)
+ // This is to preserve backward-compatibility
+ if (zkServerAddress == null || zkServerAddress.isEmpty()) {
+ throw new IllegalArgumentException("ZK server address is null or empty!");
+ }
+ zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(_zkServerAddress));
+ zkClient.setZkSerializer(new ZNRecordSerializer());
+ }
+
+ _zkClient = zkClient;
_admin = new ZKHelixAdmin(_zkClient);
_usesExternalZkClient = false;
}
- public ClusterSetup(HelixZkClient zkClient) {
+ public ClusterSetup(RealmAwareZkClient zkClient) {
_zkServerAddress = zkClient.getServers();
_zkClient = zkClient;
_admin = new ZKHelixAdmin(_zkClient);
_usesExternalZkClient = true;
}
- public ClusterSetup(HelixZkClient zkClient, HelixAdmin zkHelixAdmin) {
+ public ClusterSetup(RealmAwareZkClient zkClient, HelixAdmin zkHelixAdmin) {
_zkServerAddress = zkClient.getServers();
_zkClient = zkClient;
_admin = zkHelixAdmin;
_usesExternalZkClient = true;
}
+ private ClusterSetup(Builder builder) throws IOException, InvalidRoutingDataException {
+ switch (builder._realmMode) {
+ case MULTI_REALM:
Review comment:
If it fails, shall we fall it back to connect to single ZK?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r389157867
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -135,37 +138,74 @@
public static final String removeConstraint = "removeConstraint";
private static final Logger _logger = LoggerFactory.getLogger(ClusterSetup.class);
- private final String _zkServerAddress;
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ private String _zkServerAddress;
+ private final RealmAwareZkClient _zkClient;
+ // true if ZkBaseDataAccessor was instantiated with a RealmAwareZkClient, false otherwise
// This is used for close() to determine how ZkBaseDataAccessor should close the underlying
// ZkClient
private final boolean _usesExternalZkClient;
private final HelixAdmin _admin;
public ClusterSetup(String zkServerAddress) {
_zkServerAddress = zkServerAddress;
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(_zkServerAddress));
- _zkClient.setZkSerializer(new ZNRecordSerializer());
+
+ // First, try to start on multi-realm mode using FederatedZkClient
+ RealmAwareZkClient zkClient;
+ try {
+ zkClient = new FederatedZkClient(
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(),
+ new RealmAwareZkClient.RealmAwareZkClientConfig());
+ } catch (InvalidRoutingDataException | IOException | IllegalStateException e) {
+ // Note: IllegalStateException is for HttpRoutingDataReader if MSDS endpoint cannot be found
+ // Fall back to single-realm mode using SharedZkClient (HelixZkClient)
+ // This is to preserve backward-compatibility
+ if (zkServerAddress == null || zkServerAddress.isEmpty()) {
+ throw new IllegalArgumentException("ZK server address is null or empty!");
+ }
+ zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(_zkServerAddress));
+ zkClient.setZkSerializer(new ZNRecordSerializer());
+ }
+
+ _zkClient = zkClient;
_admin = new ZKHelixAdmin(_zkClient);
_usesExternalZkClient = false;
}
- public ClusterSetup(HelixZkClient zkClient) {
+ public ClusterSetup(RealmAwareZkClient zkClient) {
_zkServerAddress = zkClient.getServers();
_zkClient = zkClient;
_admin = new ZKHelixAdmin(_zkClient);
_usesExternalZkClient = true;
}
- public ClusterSetup(HelixZkClient zkClient, HelixAdmin zkHelixAdmin) {
+ public ClusterSetup(RealmAwareZkClient zkClient, HelixAdmin zkHelixAdmin) {
_zkServerAddress = zkClient.getServers();
_zkClient = zkClient;
_admin = zkHelixAdmin;
_usesExternalZkClient = true;
}
+ private ClusterSetup(Builder builder) throws IOException, InvalidRoutingDataException {
+ switch (builder._realmMode) {
+ case MULTI_REALM:
Review comment:
Discussed offline. We shouldn't fall back on multi-realm mode.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r391070289
##########
File path: helix-common/src/main/java/org/apache/helix/SystemPropertyKeys.java
##########
@@ -57,4 +60,7 @@
// MBean monitor for helix.
public static final String HELIX_MONITOR_TIME_WINDOW_LENGTH_MS = "helix.monitor.slidingTimeWindow.ms";
+
+ // Multi-ZK mode enable/disable flag
+ public static final String MULTI_ZK_ENABLED = "helix.multiZkEnabled";
Review comment:
I understand there is no rule for naming convention for a property key. But are we considering xxx.yyy.zzz in lowercase ? I would prefer this way as it is more readable, commonly used and easier to troubleshoot. I prefer this way. Just 2 cents
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on issue #861: Make ClusterSetup
realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on issue #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#issuecomment-597976405
This PR is ready to be merged, approved by @dasahcc
We make ClusterSetup, a Helix Java API, realm-aware so that this could be used in a multi-ZK environment.
Changelist:
Add a Builder to enable users to set internal ZkClient parameters
Add the realm-aware behavior in existing constructors
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #861: Make
ClusterSetup realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #861: Make ClusterSetup realm-aware
URL: https://github.com/apache/helix/pull/861#discussion_r389182244
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -78,6 +78,7 @@
import org.apache.helix.util.HelixUtil;
import org.apache.helix.util.RebalanceUtil;
import org.apache.helix.zookeeper.api.client.HelixZkClient;
Review comment:
Needed. HelixZkClient is still 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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org