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/04 07:56:42 UTC
[GitHub] [helix] pkuwm opened a new pull request #855: [WIP] Make
ZkBaseDataAccessor realm-aware
pkuwm opened a new pull request #855: [WIP] Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855
Draft: to add a test for multi-realm mode.
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Implements #854
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
To make Helix Java APIs realm-aware, we need to make ZkBaseDataAccessor realm-aware to use FederatedZkClient for multi-realm mode.
### Tests
- [ ] The following tests are written for this issue:
- (to be added...)
- [ ] 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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r388080036
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -1248,4 +1240,123 @@ public void close() {
_zkClient.close();
}
}
+
+ public static class Builder {
+ private String zkAddress;
+ private RealmAwareZkClient.RealmMode realmMode;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig;
+
+ public Builder() {
+ }
+
+ public ZkBaseDataAccessor.Builder setZkAddress(String zkAddress) {
+ this.zkAddress = zkAddress;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ this.realmMode = realmMode;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ this.realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ this.realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return this;
+ }
+
+ public ZkBaseDataAccessor<?> build() throws Exception {
+ validate();
+ return new ZkBaseDataAccessor<>(this);
+ }
+
+ /**
+ * Validate the given parameters before creating an instance of ConfigAccessor.
+ */
+ private void validate() {
Review comment:
Ideally, validation logic should also resolve things like ZkClientType (if it's not set), so that the constructor should only worry about "constructing", not figuring out what's set or not. Let's try to separate logic as much as we can - this will help future development much easier in my opinion.
----------------------------------------------------------------
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r389205674
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -102,21 +108,75 @@ public AccessResult() {
private static Logger LOG = LoggerFactory.getLogger(ZkBaseDataAccessor.class);
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ 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;
+ /**
+ * @deprecated it is recommended to use the other constructors instead to avoid having to manually
+ * create and maintain a RealmAwareZkClient outside of ZkBaseDataAccessor.
+ *
+ * @param zkClient A created RealmAwareZkClient
+ */
@Deprecated
- public ZkBaseDataAccessor(HelixZkClient zkClient) {
+ public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
Review comment:
Do we have a PR for HelixDataAccessor as well?
----------------------------------------------------------------
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r389205958
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -1248,4 +1289,122 @@ public void close() {
_zkClient.close();
}
}
+
+ public static class Builder {
+ private String zkAddress;
+ private RealmAwareZkClient.RealmMode realmMode;
+ private ZkBaseDataAccessor.ZkClientType zkClientType;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig;
+
+ public Builder() {
+ }
+
+ public ZkBaseDataAccessor.Builder setZkAddress(String zkAddress) {
+ this.zkAddress = zkAddress;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ this.realmMode = realmMode;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setZkClientType(
+ ZkBaseDataAccessor.ZkClientType zkClientType) {
+ this.zkClientType = zkClientType;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ this.realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ this.realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return this;
+ }
+
+ public ZkBaseDataAccessor<?> build() throws Exception {
+ validate();
+ return new ZkBaseDataAccessor<>(this);
+ }
+
+ /**
+ * Validate the given parameters before creating an instance of ConfigAccessor.
+ */
+ private void validate() {
+ // Resolve RealmMode based on other parameters
+ boolean isZkAddressSet = zkAddress != null && !zkAddress.isEmpty();
+ boolean isZkClientTypeSet = zkClientType != null;
+
+ if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM && !isZkAddressSet) {
+ throw new HelixException(
+ "RealmMode cannot be single-realm without a valid ZkAddress set!");
+ }
+ if (realmMode == null) {
+ realmMode = isZkAddressSet ? RealmAwareZkClient.RealmMode.SINGLE_REALM
+ : RealmAwareZkClient.RealmMode.MULTI_REALM;
+ }
+
+ // 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, default to SHARED
+ if (!isZkClientTypeSet) {
+ zkClientType = ZkBaseDataAccessor.ZkClientType.SHARED;
+ }
+
+ // Resolve RealmAwareZkClientConfig
+ boolean isZkClientConfigSet = realmAwareZkClientConfig != null;
+ // Resolve which clientConfig to use
+ realmAwareZkClientConfig =
+ isZkClientConfigSet ? realmAwareZkClientConfig.createHelixZkClientConfig()
+ : new HelixZkClient.ZkClientConfig().setZkSerializer(new ZNRecordSerializer());
+
+ // Resolve RealmAwareZkConnectionConfig
+ if (realmAwareZkConnectionConfig == null) {
+ // If not set, create a default one
+ realmAwareZkConnectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
+ }
+ }
+ }
+
+ private RealmAwareZkClient buildRealmAwareZkClient(
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig, String zkAddress,
+ ZkClientType zkClientType) {
+ try {
+ return new FederatedZkClient(
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(), clientConfig);
+ } catch (IllegalStateException | IOException | InvalidRoutingDataException e) {
+ // Fall back to connect on single-realm mode if failed to connect on multi-realm mode and
+ // ZK address is not empty.
+ LOG.info("Not able to connect on multi-realm mode, caused by: {}. "
+ + "Connecting on single-realm mode to ZK: {}.", e.getMessage(), zkAddress);
+ }
+
+ RealmAwareZkClient zkClient;
+
+ switch (zkClientType) {
+ case DEDICATED:
+ zkClient = DedicatedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
+ clientConfig.createHelixZkClientConfig());
+ break;
+ case SHARED:
+ default:
+ zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
+ clientConfig.createHelixZkClientConfig());
+ break;
Review comment:
Is this break necessary?
----------------------------------------------------------------
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r390794981
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -102,28 +118,85 @@ public AccessResult() {
private static Logger LOG = LoggerFactory.getLogger(ZkBaseDataAccessor.class);
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ 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;
+ /**
+ * @deprecated it is recommended to use the builder constructor {@link Builder}
+ * instead to avoid having to manually create and maintain a RealmAwareZkClient
+ * outside of ZkBaseDataAccessor.
+ *
+ * @param zkClient A created RealmAwareZkClient
+ */
@Deprecated
- public ZkBaseDataAccessor(HelixZkClient zkClient) {
+ public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
if (zkClient == null) {
throw new NullPointerException("zkclient is null");
}
_zkClient = zkClient;
_usesExternalZkClient = true;
}
+ private ZkBaseDataAccessor(Builder builder) {
+ switch (builder.realmMode) {
+ case MULTI_REALM:
+ try {
+ if (builder.zkClientType == ZkBaseDataAccessor.ZkClientType.DEDICATED) {
+ // Use a realm-aware dedicated zk client
+ _zkClient = DedicatedZkClientFactory.getInstance()
+ .buildZkClient(builder.realmAwareZkConnectionConfig,
+ builder.realmAwareZkClientConfig);
+ } else if (builder.zkClientType == ZkBaseDataAccessor.ZkClientType.SHARED) {
Review comment:
Nit: This is already inside ZkBaseDataAccessor, so no need to qualify using ZkBaseDataAccessor. Did you copy the code from the other PRs? IDE automatically puts these things.
----------------------------------------------------------------
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r389206433
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -1248,4 +1289,122 @@ public void close() {
_zkClient.close();
}
}
+
+ public static class Builder {
+ private String zkAddress;
+ private RealmAwareZkClient.RealmMode realmMode;
+ private ZkBaseDataAccessor.ZkClientType zkClientType;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig;
+
+ public Builder() {
+ }
+
+ public ZkBaseDataAccessor.Builder setZkAddress(String zkAddress) {
+ this.zkAddress = zkAddress;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ this.realmMode = realmMode;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setZkClientType(
+ ZkBaseDataAccessor.ZkClientType zkClientType) {
+ this.zkClientType = zkClientType;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ this.realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ this.realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return this;
+ }
+
+ public ZkBaseDataAccessor<?> build() throws Exception {
+ validate();
+ return new ZkBaseDataAccessor<>(this);
+ }
+
+ /**
+ * Validate the given parameters before creating an instance of ConfigAccessor.
+ */
+ private void validate() {
+ // Resolve RealmMode based on other parameters
+ boolean isZkAddressSet = zkAddress != null && !zkAddress.isEmpty();
+ boolean isZkClientTypeSet = zkClientType != null;
+
+ if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM && !isZkAddressSet) {
+ throw new HelixException(
+ "RealmMode cannot be single-realm without a valid ZkAddress set!");
+ }
+ if (realmMode == null) {
+ realmMode = isZkAddressSet ? RealmAwareZkClient.RealmMode.SINGLE_REALM
+ : RealmAwareZkClient.RealmMode.MULTI_REALM;
+ }
+
+ // 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, default to SHARED
+ if (!isZkClientTypeSet) {
+ zkClientType = ZkBaseDataAccessor.ZkClientType.SHARED;
+ }
+
+ // Resolve RealmAwareZkClientConfig
+ boolean isZkClientConfigSet = realmAwareZkClientConfig != null;
+ // Resolve which clientConfig to use
+ realmAwareZkClientConfig =
+ isZkClientConfigSet ? realmAwareZkClientConfig.createHelixZkClientConfig()
+ : new HelixZkClient.ZkClientConfig().setZkSerializer(new ZNRecordSerializer());
+
+ // Resolve RealmAwareZkConnectionConfig
+ if (realmAwareZkConnectionConfig == null) {
+ // If not set, create a default one
+ realmAwareZkConnectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
+ }
+ }
+ }
+
+ private RealmAwareZkClient buildRealmAwareZkClient(
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig, String zkAddress,
+ ZkClientType zkClientType) {
+ try {
+ return new FederatedZkClient(
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(), clientConfig);
+ } catch (IllegalStateException | IOException | InvalidRoutingDataException e) {
+ // Fall back to connect on single-realm mode if failed to connect on multi-realm mode and
+ // ZK address is not empty.
+ LOG.info("Not able to connect on multi-realm mode, caused by: {}. "
+ + "Connecting on single-realm mode to ZK: {}.", e.getMessage(), zkAddress);
Review comment:
Great use of logging, but let's also note here that this is for backward-compatibility for constructors that take in zkaddress or zkclient.
----------------------------------------------------------------
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r389826601
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -1248,4 +1280,127 @@ public void close() {
_zkClient.close();
}
}
+
+ public static class Builder {
+ private String zkAddress;
+ private RealmAwareZkClient.RealmMode realmMode;
+ private ZkBaseDataAccessor.ZkClientType zkClientType;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig;
+
+ public Builder() {
+ }
+
+ public ZkBaseDataAccessor.Builder setZkAddress(String zkAddress) {
+ this.zkAddress = zkAddress;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ this.realmMode = realmMode;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setZkClientType(
+ ZkBaseDataAccessor.ZkClientType zkClientType) {
+ this.zkClientType = zkClientType;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ this.realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ this.realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return this;
+ }
+
+ public ZkBaseDataAccessor<?> build() throws Exception {
+ validate();
+ return new ZkBaseDataAccessor<>(this);
+ }
+
+ /**
+ * Validate the given parameters before creating an instance of ConfigAccessor.
Review comment:
Change comment.
----------------------------------------------------------------
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r389317008
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -1248,4 +1289,122 @@ public void close() {
_zkClient.close();
}
}
+
+ public static class Builder {
+ private String zkAddress;
+ private RealmAwareZkClient.RealmMode realmMode;
+ private ZkBaseDataAccessor.ZkClientType zkClientType;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig;
+
+ public Builder() {
+ }
+
+ public ZkBaseDataAccessor.Builder setZkAddress(String zkAddress) {
+ this.zkAddress = zkAddress;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ this.realmMode = realmMode;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setZkClientType(
+ ZkBaseDataAccessor.ZkClientType zkClientType) {
+ this.zkClientType = zkClientType;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ this.realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ this.realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return this;
+ }
+
+ public ZkBaseDataAccessor<?> build() throws Exception {
+ validate();
+ return new ZkBaseDataAccessor<>(this);
+ }
+
+ /**
+ * Validate the given parameters before creating an instance of ConfigAccessor.
+ */
+ private void validate() {
+ // Resolve RealmMode based on other parameters
+ boolean isZkAddressSet = zkAddress != null && !zkAddress.isEmpty();
+ boolean isZkClientTypeSet = zkClientType != null;
+
+ if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM && !isZkAddressSet) {
+ throw new HelixException(
+ "RealmMode cannot be single-realm without a valid ZkAddress set!");
+ }
+ if (realmMode == null) {
+ realmMode = isZkAddressSet ? RealmAwareZkClient.RealmMode.SINGLE_REALM
+ : RealmAwareZkClient.RealmMode.MULTI_REALM;
+ }
+
+ // 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, default to SHARED
+ if (!isZkClientTypeSet) {
+ zkClientType = ZkBaseDataAccessor.ZkClientType.SHARED;
+ }
+
+ // Resolve RealmAwareZkClientConfig
+ boolean isZkClientConfigSet = realmAwareZkClientConfig != null;
+ // Resolve which clientConfig to use
+ realmAwareZkClientConfig =
+ isZkClientConfigSet ? realmAwareZkClientConfig.createHelixZkClientConfig()
+ : new HelixZkClient.ZkClientConfig().setZkSerializer(new ZNRecordSerializer());
+
+ // Resolve RealmAwareZkConnectionConfig
+ if (realmAwareZkConnectionConfig == null) {
+ // If not set, create a default one
+ realmAwareZkConnectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
+ }
+ }
+ }
+
+ private RealmAwareZkClient buildRealmAwareZkClient(
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig, String zkAddress,
+ ZkClientType zkClientType) {
+ try {
+ return new FederatedZkClient(
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(), clientConfig);
+ } catch (IllegalStateException | IOException | InvalidRoutingDataException e) {
+ // Fall back to connect on single-realm mode if failed to connect on multi-realm mode and
+ // ZK address is not empty.
+ LOG.info("Not able to connect on multi-realm mode, caused by: {}. "
+ + "Connecting on single-realm mode to ZK: {}.", e.getMessage(), zkAddress);
+ }
+
+ RealmAwareZkClient zkClient;
+
+ switch (zkClientType) {
+ case DEDICATED:
+ zkClient = DedicatedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
+ clientConfig.createHelixZkClientConfig());
+ break;
+ case SHARED:
+ default:
+ zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
+ clientConfig.createHelixZkClientConfig());
+ break;
Review comment:
It is just my habit: As a matter of good form, put a break after the last case (the default here) even though it's logically unnecessary. Some day when another case gets added at the end, this bit of defensive programming will save you.
----------------------------------------------------------------
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r391045595
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -198,21 +271,17 @@ public ZkBaseDataAccessor(String zkAddress, ZkSerializer zkSerializer,
* @param zkAddress
* @param pathBasedZkSerializer
* @param zkClientType
+ *
+ * @deprecated it is recommended to use the builder constructor {@link Builder}
*/
- public ZkBaseDataAccessor(String zkAddress, PathBasedZkSerializer pathBasedZkSerializer,
+ @Deprecated
+ public ZkBaseDataAccessor(String zkAddress,
+ org.apache.helix.zookeeper.zkclient.serialize.PathBasedZkSerializer pathBasedZkSerializer,
Review comment:
It is to reduce duplicate code for 2 constructors that have serializer parameters. `BasicZkSerializer` implements the long path `PathBasedZkSerializer` interface in zookeeper module. So we need this long path zookeeper `PathBasedZkSerializer`.
```
public ZkBaseDataAccessor(String zkAddress, ZkSerializer zkSerializer, ZkClientType zkClientType) {
this(zkAddress, new BasicZkSerializer(zkSerializer), zkClientType);
}
```
Cons: `new BasicZkSerializer(zkSerializer)` just follows the behavior in ZkClientConfig. If it is changed in ZkClientConfig to use another instance other than BasicZkSerializer, the one here would not follow that change.
Well, if we don't want to reduce the duplicate code, we can just keep the original code.
----------------------------------------------------------------
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 issue #855: Make ZkBaseDataAccessor
realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on issue #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#issuecomment-598014767
This PR is ready to be merged, approved by @narendly
```
This commits makes ZkBaseDataAccessor realm-aware by building according realm-aware ZkClients in the constructor. A Builder is provided to set realm-aware client config and connection config.
```
----------------------------------------------------------------
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r391347427
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -198,21 +271,17 @@ public ZkBaseDataAccessor(String zkAddress, ZkSerializer zkSerializer,
* @param zkAddress
* @param pathBasedZkSerializer
* @param zkClientType
+ *
+ * @deprecated it is recommended to use the builder constructor {@link Builder}
*/
- public ZkBaseDataAccessor(String zkAddress, PathBasedZkSerializer pathBasedZkSerializer,
+ @Deprecated
+ public ZkBaseDataAccessor(String zkAddress,
+ org.apache.helix.zookeeper.zkclient.serialize.PathBasedZkSerializer pathBasedZkSerializer,
Review comment:
I'd prefer to leave it as is and tackle this later. This sort of refactoring doesn't have to be in the scope of this PR. Having 2 constructors is fine for now.
----------------------------------------------------------------
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r390732665
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -199,20 +238,13 @@ public ZkBaseDataAccessor(String zkAddress, ZkSerializer zkSerializer,
* @param pathBasedZkSerializer
* @param zkClientType
*/
- public ZkBaseDataAccessor(String zkAddress, PathBasedZkSerializer pathBasedZkSerializer,
+ public ZkBaseDataAccessor(String zkAddress,
Review comment:
As discussed offline, we don't want the fallback behavior and also make these old constructors 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:
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] dasahcc commented on a change in pull request #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r389827888
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -199,20 +238,13 @@ public ZkBaseDataAccessor(String zkAddress, ZkSerializer zkSerializer,
* @param pathBasedZkSerializer
* @param zkClientType
*/
- public ZkBaseDataAccessor(String zkAddress, PathBasedZkSerializer pathBasedZkSerializer,
+ public ZkBaseDataAccessor(String zkAddress,
Review comment:
I think we should make the behavior description in the Java 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:
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r389205854
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -102,21 +108,75 @@ public AccessResult() {
private static Logger LOG = LoggerFactory.getLogger(ZkBaseDataAccessor.class);
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ 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;
+ /**
+ * @deprecated it is recommended to use the other constructors instead to avoid having to manually
+ * create and maintain a RealmAwareZkClient outside of ZkBaseDataAccessor.
+ *
+ * @param zkClient A created RealmAwareZkClient
+ */
@Deprecated
- public ZkBaseDataAccessor(HelixZkClient zkClient) {
+ public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
if (zkClient == null) {
throw new NullPointerException("zkclient is null");
}
_zkClient = zkClient;
_usesExternalZkClient = true;
}
+ private ZkBaseDataAccessor(Builder builder) {
+ RealmAwareZkClient zkClient;
+ switch (builder.realmMode) {
+ case MULTI_REALM:
+ try {
+ zkClient = new FederatedZkClient(builder.realmAwareZkConnectionConfig,
+ builder.realmAwareZkClientConfig);
+ // Break here to exit.
+ break;
+ } catch (IOException | InvalidRoutingDataException | IllegalStateException e) {
+ if (builder.zkAddress == null || builder.zkAddress.isEmpty()) {
+ throw new IllegalStateException("Not able to connect on multi-realm mode.", e);
+ }
+ LOG.info("Not able to connect on multi-realm mode. "
+ + "Connecting on single-realm mode to ZK: {}", builder.zkAddress);
+ builder.setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM);
+ }
+ // No break here. If connecting on multi-realm fails and ZK address is valid, connecting
+ // on single-realm is allowed.
+
Review comment:
As discussed offline, please make sure you're throwing a HelixException and propagating the Exception.
Note that we shouldn't fall back to single-realm mode. We should fail 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:
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r388079805
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -1248,4 +1240,123 @@ public void close() {
_zkClient.close();
}
}
+
+ public static class Builder {
+ private String zkAddress;
+ private RealmAwareZkClient.RealmMode realmMode;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig;
+
Review comment:
We'd need ZkBaseDataAccessor-specific parameters here as well such as ZkClientType (See https://github.com/apache/helix/pull/863 for reference)
----------------------------------------------------------------
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] dasahcc commented on a change in pull request #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r391315716
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -102,28 +118,85 @@ public AccessResult() {
private static Logger LOG = LoggerFactory.getLogger(ZkBaseDataAccessor.class);
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ 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;
+ /**
+ * @deprecated it is recommended to use the builder constructor {@link Builder}
+ * instead to avoid having to manually create and maintain a RealmAwareZkClient
+ * outside of ZkBaseDataAccessor.
+ *
+ * @param zkClient A created RealmAwareZkClient
+ */
@Deprecated
- public ZkBaseDataAccessor(HelixZkClient zkClient) {
+ public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
if (zkClient == null) {
throw new NullPointerException("zkclient is null");
}
_zkClient = zkClient;
_usesExternalZkClient = true;
}
+ private ZkBaseDataAccessor(Builder builder) {
+ switch (builder.realmMode) {
+ case MULTI_REALM:
+ try {
+ if (builder.zkClientType == ZkClientType.DEDICATED) {
+ // Use a realm-aware dedicated zk client
+ _zkClient = DedicatedZkClientFactory.getInstance()
+ .buildZkClient(builder.realmAwareZkConnectionConfig,
+ builder.realmAwareZkClientConfig);
+ } else if (builder.zkClientType == ZkClientType.SHARED) {
+ // Use a realm-aware shared zk client
+ _zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(builder.realmAwareZkConnectionConfig,
+ builder.realmAwareZkClientConfig);
+ } else {
Review comment:
So we do agree to support shared zkclient while in 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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r391324712
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -102,28 +118,85 @@ public AccessResult() {
private static Logger LOG = LoggerFactory.getLogger(ZkBaseDataAccessor.class);
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ 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;
+ /**
+ * @deprecated it is recommended to use the builder constructor {@link Builder}
+ * instead to avoid having to manually create and maintain a RealmAwareZkClient
+ * outside of ZkBaseDataAccessor.
+ *
+ * @param zkClient A created RealmAwareZkClient
+ */
@Deprecated
- public ZkBaseDataAccessor(HelixZkClient zkClient) {
+ public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
if (zkClient == null) {
throw new NullPointerException("zkclient is null");
}
_zkClient = zkClient;
_usesExternalZkClient = true;
}
+ private ZkBaseDataAccessor(Builder builder) {
+ switch (builder.realmMode) {
+ case MULTI_REALM:
+ try {
+ if (builder.zkClientType == ZkClientType.DEDICATED) {
+ // Use a realm-aware dedicated zk client
+ _zkClient = DedicatedZkClientFactory.getInstance()
+ .buildZkClient(builder.realmAwareZkConnectionConfig,
+ builder.realmAwareZkClientConfig);
+ } else if (builder.zkClientType == ZkClientType.SHARED) {
+ // Use a realm-aware shared zk client
+ _zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(builder.realmAwareZkConnectionConfig,
+ builder.realmAwareZkClientConfig);
+ } else {
Review comment:
This is a realm aware shared Zk client. If the user wants to use it this way, then they can
----------------------------------------------------------------
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r388079677
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -1248,4 +1240,123 @@ public void close() {
_zkClient.close();
}
}
+
+ public static class Builder {
+ private String zkAddress;
+ private RealmAwareZkClient.RealmMode realmMode;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig;
+
+ public Builder() {
+ }
+
+ public ZkBaseDataAccessor.Builder setZkAddress(String zkAddress) {
+ this.zkAddress = zkAddress;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ this.realmMode = realmMode;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ this.realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ this.realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return this;
+ }
+
+ public ZkBaseDataAccessor<?> build() throws Exception {
+ validate();
+ return new ZkBaseDataAccessor<>(this);
+ }
+
+ /**
+ * Validate the given parameters before creating an instance of ConfigAccessor.
+ */
+ private void validate() {
+ // Resolve RealmMode based on other parameters
+ boolean isZkAddressSet = zkAddress != null && !zkAddress.isEmpty();
+ if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM && !isZkAddressSet) {
+ throw new HelixException(
+ "ConfigAccessor: RealmMode cannot be single-realm without a valid ZkAddress set!");
+ }
+ if (realmMode == null) {
+ realmMode = isZkAddressSet ? RealmAwareZkClient.RealmMode.SINGLE_REALM
+ : RealmAwareZkClient.RealmMode.MULTI_REALM;
+ }
+
+ // Resolve RealmAwareZkClientConfig
+ boolean isZkClientConfigSet = realmAwareZkClientConfig != null;
+ // Resolve which clientConfig to use
+ realmAwareZkClientConfig =
+ isZkClientConfigSet ? realmAwareZkClientConfig.createHelixZkClientConfig()
+ : new HelixZkClient.ZkClientConfig().setZkSerializer(new ZNRecordSerializer());
+
+ // Resolve RealmAwareZkConnectionConfig
+ if (realmAwareZkConnectionConfig == null) {
+ // If not set, create a default one
+ realmAwareZkConnectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
+ }
+ }
+ }
+
+ private ZkBaseDataAccessor(Builder builder) throws IOException, InvalidRoutingDataException {
Review comment:
Could we move up this constructor so it's with other public 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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r387968735
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -1248,4 +1251,41 @@ public void close() {
_zkClient.close();
}
}
+
+ private RealmAwareZkClient buildRealmAwareZkClient(
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig, String zkAddress,
+ ZkClientType zkClientType) {
+ RealmAwareZkClient zkClient;
+
+ try {
+ zkClient = new FederatedZkClient(
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(), clientConfig);
+ } catch (IllegalStateException | IllegalArgumentException | IOException | InvalidRoutingDataException e) {
Review comment:
Under what circumstances will `IllegalArgumentException` be thrown?
----------------------------------------------------------------
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r389206299
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -1248,4 +1289,122 @@ public void close() {
_zkClient.close();
}
}
+
+ public static class Builder {
+ private String zkAddress;
+ private RealmAwareZkClient.RealmMode realmMode;
+ private ZkBaseDataAccessor.ZkClientType zkClientType;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig;
+
+ public Builder() {
+ }
+
+ public ZkBaseDataAccessor.Builder setZkAddress(String zkAddress) {
+ this.zkAddress = zkAddress;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ this.realmMode = realmMode;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setZkClientType(
+ ZkBaseDataAccessor.ZkClientType zkClientType) {
+ this.zkClientType = zkClientType;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ this.realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return this;
+ }
+
+ public ZkBaseDataAccessor.Builder setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ this.realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return this;
+ }
+
+ public ZkBaseDataAccessor<?> build() throws Exception {
+ validate();
+ return new ZkBaseDataAccessor<>(this);
+ }
+
+ /**
+ * Validate the given parameters before creating an instance of ConfigAccessor.
+ */
+ private void validate() {
+ // Resolve RealmMode based on other parameters
+ boolean isZkAddressSet = zkAddress != null && !zkAddress.isEmpty();
+ boolean isZkClientTypeSet = zkClientType != null;
+
+ if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM && !isZkAddressSet) {
+ throw new HelixException(
+ "RealmMode cannot be single-realm without a valid ZkAddress set!");
+ }
+ if (realmMode == null) {
+ realmMode = isZkAddressSet ? RealmAwareZkClient.RealmMode.SINGLE_REALM
+ : RealmAwareZkClient.RealmMode.MULTI_REALM;
+ }
+
+ // 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, default to SHARED
+ if (!isZkClientTypeSet) {
+ zkClientType = ZkBaseDataAccessor.ZkClientType.SHARED;
+ }
+
+ // Resolve RealmAwareZkClientConfig
+ boolean isZkClientConfigSet = realmAwareZkClientConfig != null;
+ // Resolve which clientConfig to use
+ realmAwareZkClientConfig =
+ isZkClientConfigSet ? realmAwareZkClientConfig.createHelixZkClientConfig()
+ : new HelixZkClient.ZkClientConfig().setZkSerializer(new ZNRecordSerializer());
+
+ // Resolve RealmAwareZkConnectionConfig
+ if (realmAwareZkConnectionConfig == null) {
+ // If not set, create a default one
+ realmAwareZkConnectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
+ }
+ }
+ }
+
+ private RealmAwareZkClient buildRealmAwareZkClient(
Review comment:
Let's clearly note that this private method is only to be used for constructors that do **not** take a Builder in as a parameter because of the fallback 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:
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r387968312
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -102,14 +107,31 @@ public AccessResult() {
private static Logger LOG = LoggerFactory.getLogger(ZkBaseDataAccessor.class);
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ private static final String EMPTY_ZK_ADDRESS = "";
+
+ 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;
+ /**
+ * This default constructor attempts to connect to ZK on multi-realm mode.
+ *
+ * @exception IllegalStateException if connecting to ZK on multi-realm mode fails
+ */
+ public ZkBaseDataAccessor() {
+ this(EMPTY_ZK_ADDRESS);
+ }
Review comment:
ZkBaseDataAccessor needs to be able to take in Connection and Client configs. Instead of a constructor that takes in 0 parameters, could we use the builder pattern to achieve this? See the example for ConfigAccessor.
----------------------------------------------------------------
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r390795448
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -198,21 +271,17 @@ public ZkBaseDataAccessor(String zkAddress, ZkSerializer zkSerializer,
* @param zkAddress
* @param pathBasedZkSerializer
* @param zkClientType
+ *
+ * @deprecated it is recommended to use the builder constructor {@link Builder}
*/
- public ZkBaseDataAccessor(String zkAddress, PathBasedZkSerializer pathBasedZkSerializer,
+ @Deprecated
+ public ZkBaseDataAccessor(String zkAddress,
+ org.apache.helix.zookeeper.zkclient.serialize.PathBasedZkSerializer pathBasedZkSerializer,
Review comment:
why do we need the long package name for this serializer?
----------------------------------------------------------------
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r389318885
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -102,21 +108,75 @@ public AccessResult() {
private static Logger LOG = LoggerFactory.getLogger(ZkBaseDataAccessor.class);
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ 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;
+ /**
+ * @deprecated it is recommended to use the other constructors instead to avoid having to manually
+ * create and maintain a RealmAwareZkClient outside of ZkBaseDataAccessor.
+ *
+ * @param zkClient A created RealmAwareZkClient
+ */
@Deprecated
- public ZkBaseDataAccessor(HelixZkClient zkClient) {
+ public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
if (zkClient == null) {
throw new NullPointerException("zkclient is null");
}
_zkClient = zkClient;
_usesExternalZkClient = true;
}
+ private ZkBaseDataAccessor(Builder builder) {
+ RealmAwareZkClient zkClient;
+ switch (builder.realmMode) {
+ case MULTI_REALM:
+ try {
+ zkClient = new FederatedZkClient(builder.realmAwareZkConnectionConfig,
+ builder.realmAwareZkClientConfig);
+ // Break here to exit.
+ break;
+ } catch (IOException | InvalidRoutingDataException | IllegalStateException e) {
+ if (builder.zkAddress == null || builder.zkAddress.isEmpty()) {
+ throw new IllegalStateException("Not able to connect on multi-realm mode.", e);
+ }
+ LOG.info("Not able to connect on multi-realm mode. "
+ + "Connecting on single-realm mode to ZK: {}", builder.zkAddress);
+ builder.setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM);
+ }
+ // No break here. If connecting on multi-realm fails and ZK address is valid, connecting
+ // on single-realm is allowed.
+
Review comment:
Done.
----------------------------------------------------------------
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r389318882
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -102,21 +108,75 @@ public AccessResult() {
private static Logger LOG = LoggerFactory.getLogger(ZkBaseDataAccessor.class);
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ 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;
+ /**
+ * @deprecated it is recommended to use the other constructors instead to avoid having to manually
+ * create and maintain a RealmAwareZkClient outside of ZkBaseDataAccessor.
+ *
+ * @param zkClient A created RealmAwareZkClient
+ */
@Deprecated
- public ZkBaseDataAccessor(HelixZkClient zkClient) {
+ public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
Review comment:
As we discussed offline, we decided not to add a builder constructor for HelixDataAccessor.
The reason is: Underneath it is using a BaseDataAccessor, which could an instance of ZkBaseDataAccessor/ZkCacheBaseDataAccessor/HelixPropertyStore, etc.. If we offer a builder constructor, we have to determine which instance of BaseDataAccessor. If we just offer a builder constructor like this Builder().setBaseDataAccessor(), I don’t think we need it.
----------------------------------------------------------------
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r391327397
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -102,28 +118,85 @@ public AccessResult() {
private static Logger LOG = LoggerFactory.getLogger(ZkBaseDataAccessor.class);
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ 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;
+ /**
+ * @deprecated it is recommended to use the builder constructor {@link Builder}
+ * instead to avoid having to manually create and maintain a RealmAwareZkClient
+ * outside of ZkBaseDataAccessor.
+ *
+ * @param zkClient A created RealmAwareZkClient
+ */
@Deprecated
- public ZkBaseDataAccessor(HelixZkClient zkClient) {
+ public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
if (zkClient == null) {
throw new NullPointerException("zkclient is null");
}
_zkClient = zkClient;
_usesExternalZkClient = true;
}
+ private ZkBaseDataAccessor(Builder builder) {
+ switch (builder.realmMode) {
+ case MULTI_REALM:
+ try {
+ if (builder.zkClientType == ZkClientType.DEDICATED) {
+ // Use a realm-aware dedicated zk client
+ _zkClient = DedicatedZkClientFactory.getInstance()
+ .buildZkClient(builder.realmAwareZkConnectionConfig,
+ builder.realmAwareZkClientConfig);
+ } else if (builder.zkClientType == ZkClientType.SHARED) {
+ // Use a realm-aware shared zk client
+ _zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(builder.realmAwareZkConnectionConfig,
+ builder.realmAwareZkClientConfig);
+ } else {
Review comment:
@dasahcc This is a realm-aware shared ZkClient. Users could construct the client with the sharding key 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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r388686792
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -1248,4 +1240,123 @@ public void close() {
_zkClient.close();
}
}
+
+ public static class Builder {
+ private String zkAddress;
+ private RealmAwareZkClient.RealmMode realmMode;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig;
+
Review comment:
Got it.
----------------------------------------------------------------
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 #855: Make
ZkBaseDataAccessor realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855#discussion_r390794981
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -102,28 +118,85 @@ public AccessResult() {
private static Logger LOG = LoggerFactory.getLogger(ZkBaseDataAccessor.class);
- private final HelixZkClient _zkClient;
- // true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
+ 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;
+ /**
+ * @deprecated it is recommended to use the builder constructor {@link Builder}
+ * instead to avoid having to manually create and maintain a RealmAwareZkClient
+ * outside of ZkBaseDataAccessor.
+ *
+ * @param zkClient A created RealmAwareZkClient
+ */
@Deprecated
- public ZkBaseDataAccessor(HelixZkClient zkClient) {
+ public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
if (zkClient == null) {
throw new NullPointerException("zkclient is null");
}
_zkClient = zkClient;
_usesExternalZkClient = true;
}
+ private ZkBaseDataAccessor(Builder builder) {
+ switch (builder.realmMode) {
+ case MULTI_REALM:
+ try {
+ if (builder.zkClientType == ZkBaseDataAccessor.ZkClientType.DEDICATED) {
+ // Use a realm-aware dedicated zk client
+ _zkClient = DedicatedZkClientFactory.getInstance()
+ .buildZkClient(builder.realmAwareZkConnectionConfig,
+ builder.realmAwareZkClientConfig);
+ } else if (builder.zkClientType == ZkBaseDataAccessor.ZkClientType.SHARED) {
Review comment:
Nit: This is already inside ZkBaseDataAccessor, so no need to qualify ZkClientTypes using ZkBaseDataAccessor. Did you copy the code from the other PRs? IDE automatically puts these things.
----------------------------------------------------------------
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 #855: Make ZkBaseDataAccessor
realm-aware
Posted by GitBox <gi...@apache.org>.
narendly merged pull request #855: Make ZkBaseDataAccessor realm-aware
URL: https://github.com/apache/helix/pull/855
----------------------------------------------------------------
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