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 04:41:27 UTC
[GitHub] [helix] narendly opened a new pull request #863: Make
ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
narendly opened a new pull request #863: Make ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
URL: https://github.com/apache/helix/pull/863
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Resolves #862
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
This commit makes both ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware by choosing the appropriate realm-aware ZkClients in the constructor. Also, we add a Builder here to give users options to set Connection config and Client config.
Note that ZkHelixPropertyStore extends CacheBaseDataAccessor so there is no change needed.
### Tests
- [x] The following tests are written for this issue:
The following tests cover ZkCacheBaseDataAccessor:
TestZkCacheAsyncOpSingleThread,TestZkCacheSyncOpSingleThread,TestWtCacheSyncOpSingleThread,TestWtCacheAsyncOpMultiThread
- [ ] 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"
### 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 #863: Make
ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #863: Make ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
URL: https://github.com/apache/helix/pull/863#discussion_r389849160
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
##########
@@ -842,4 +894,116 @@ public void close() {
_zkClient.close();
}
}
+
+ public static class Builder {
+ private String _zkAddress;
+ private RealmAwareZkClient.RealmMode _realmMode;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig _realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig _realmAwareZkClientConfig;
+
+ /** ZkCacheBaseDataAccessor-specific parameters */
+ private String _chrootPath;
+ private List<String> _wtCachePaths;
+ private List<String> _zkCachePaths;
+ private ZkBaseDataAccessor.ZkClientType _zkClientType;
+
+ public Builder() {
+ }
+
+ public Builder setZkAddress(String zkAddress) {
+ _zkAddress = zkAddress;
+ return this;
+ }
+
+ public Builder setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ _realmMode = realmMode;
+ return this;
+ }
+
+ public Builder setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ _realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return this;
+ }
+
+ public Builder setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ _realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return this;
+ }
+
+ public Builder setChrootPath(String chrootPath) {
+ _chrootPath = chrootPath;
+ return this;
+ }
+
+ public Builder setWtCachePaths(List<String> wtCachePaths) {
+ _wtCachePaths = wtCachePaths;
+ return this;
+ }
+
+ public Builder setZkCachePaths(List<String> zkCachePaths) {
+ _zkCachePaths = zkCachePaths;
+ return this;
+ }
+
+ /**
+ * Sets the ZkClientType. If this is set, ZkCacheBaseDataAccessor will be created on
+ * single-realm mode.
+ * @param zkClientType
+ * @return
+ */
Review comment:
That will throw an exception as ZkClientType only applies to the old Dedicated and Shared zkclients.
Do you see the following block of code in the validate() logic? How could I make this clearer?
```
// If ZkClientType is set, RealmMode must either be single-realm or not set.
if (isZkClientTypeSet && _realmMode == RealmAwareZkClient.RealmMode.MULTI_REALM) {
throw new HelixException(
"ZkCacheBaseDataAccessor: you cannot set ZkClientType 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 #863: Make
ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #863: Make ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
URL: https://github.com/apache/helix/pull/863#discussion_r389791303
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
##########
@@ -842,4 +894,116 @@ public void close() {
_zkClient.close();
}
}
+
+ public static class Builder {
Review comment:
@lei-xia
1. See https://github.com/apache/helix/issues/873 . We will do this once all Helix Java APIs have been implemented since this is a purely *internal* implementation detail. User-facing APIs stay the same.
2. No, please see existing constructors for the fallback logic.
----------------------------------------------------------------
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 #863: Make
ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #863: Make ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
URL: https://github.com/apache/helix/pull/863#discussion_r389847926
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
##########
@@ -146,17 +161,55 @@ public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String
_wtCachePaths = wtCachePaths;
_zkCachePaths = zkCachePaths;
- // TODO: need to make sure no overlap between wtCachePaths and zkCachePaths
- // TreeMap key is ordered by key string length, so more general (i.e. short) prefix
- // comes first
- _cacheMap = new TreeMap<>(new Comparator<String>() {
- @Override
- public int compare(String o1, String o2) {
- int len1 = o1.split("/").length;
- int len2 = o2.split("/").length;
- return len1 - len2;
- }
- });
+ start();
+ }
+
+ /**
+ * Constructor using a Builder that allows users to set connection and client configs.
+ * @param builder
+ */
+ private ZkCacheBaseDataAccessor(Builder builder) {
+ _chrootPath = builder._chrootPath;
+ _wtCachePaths = builder._wtCachePaths;
+ _zkCachePaths = builder._zkCachePaths;
+
+ RealmAwareZkClient zkClient;
+ switch (builder._realmMode) {
+ case MULTI_REALM:
+ try {
+ zkClient = new FederatedZkClient(builder._realmAwareZkConnectionConfig,
+ builder._realmAwareZkClientConfig);
+ break; // Must break out of the switch statement here
+ } catch (IOException | InvalidRoutingDataException | IllegalStateException e) {
Review comment:
It's already done in the builder's validate()?
----------------------------------------------------------------
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 #863: Make
ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #863: Make ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
URL: https://github.com/apache/helix/pull/863#discussion_r388744803
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
##########
@@ -146,17 +161,55 @@ public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String
_wtCachePaths = wtCachePaths;
_zkCachePaths = zkCachePaths;
- // TODO: need to make sure no overlap between wtCachePaths and zkCachePaths
- // TreeMap key is ordered by key string length, so more general (i.e. short) prefix
- // comes first
- _cacheMap = new TreeMap<>(new Comparator<String>() {
- @Override
- public int compare(String o1, String o2) {
- int len1 = o1.split("/").length;
- int len2 = o2.split("/").length;
- return len1 - len2;
- }
- });
+ start();
+ }
+
+ /**
+ * Constructor using a Builder that allows users to set connection and client configs.
+ * @param builder
+ */
+ private ZkCacheBaseDataAccessor(Builder builder) {
+ _chrootPath = builder._chrootPath;
+ _wtCachePaths = builder._wtCachePaths;
+ _zkCachePaths = builder._zkCachePaths;
+
+ RealmAwareZkClient zkClient;
+ switch (builder._realmMode) {
+ case MULTI_REALM:
+ try {
+ zkClient = new FederatedZkClient(builder._realmAwareZkConnectionConfig,
+ builder._realmAwareZkClientConfig);
+ break; // Must break out of the switch statement here
+ } catch (IOException | InvalidRoutingDataException | IllegalStateException e) {
Review comment:
Do we need to validate zk address before falling back to single 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] lei-xia commented on a change in pull request #863: Make
ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
Posted by GitBox <gi...@apache.org>.
lei-xia commented on a change in pull request #863: Make ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
URL: https://github.com/apache/helix/pull/863#discussion_r389823422
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
##########
@@ -842,4 +894,116 @@ public void close() {
_zkClient.close();
}
}
+
+ public static class Builder {
+ private String _zkAddress;
+ private RealmAwareZkClient.RealmMode _realmMode;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig _realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig _realmAwareZkClientConfig;
+
+ /** ZkCacheBaseDataAccessor-specific parameters */
+ private String _chrootPath;
+ private List<String> _wtCachePaths;
+ private List<String> _zkCachePaths;
+ private ZkBaseDataAccessor.ZkClientType _zkClientType;
+
+ public Builder() {
+ }
+
+ public Builder setZkAddress(String zkAddress) {
+ _zkAddress = zkAddress;
+ return this;
+ }
+
+ public Builder setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ _realmMode = realmMode;
+ return this;
+ }
+
+ public Builder setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ _realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return this;
+ }
+
+ public Builder setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ _realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return this;
+ }
+
+ public Builder setChrootPath(String chrootPath) {
+ _chrootPath = chrootPath;
+ return this;
+ }
+
+ public Builder setWtCachePaths(List<String> wtCachePaths) {
+ _wtCachePaths = wtCachePaths;
+ return this;
+ }
+
+ public Builder setZkCachePaths(List<String> zkCachePaths) {
+ _zkCachePaths = zkCachePaths;
+ return this;
+ }
+
+ /**
+ * Sets the ZkClientType. If this is set, ZkCacheBaseDataAccessor will be created on
+ * single-realm mode.
+ * @param zkClientType
+ * @return
+ */
Review comment:
This is not very clear. Do you mean whatever type I set here, it will change the API to single-realm mode? Then if call builder. setRealmMode(multi-realm).setZkClientType(type), what will happen?
----------------------------------------------------------------
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 #863: Make
ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #863: Make ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
URL: https://github.com/apache/helix/pull/863#discussion_r388564073
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
##########
@@ -58,7 +61,15 @@
protected ZkCallbackCache<T> _zkCache;
final ZkBaseDataAccessor<T> _baseAccessor;
- final Map<String, Cache<T>> _cacheMap;
+
+ // TODO: need to make sure no overlap between wtCachePaths and zkCachePaths
+ // TreeMap key is ordered by key string length, so more general (i.e. short) prefix
+ // comes first
+ final Map<String, Cache<T>> _cacheMap = new TreeMap<>((o1, o2) -> {
+ int len1 = o1.split("/").length;
+ int len2 = o2.split("/").length;
Review comment:
static constant for "/" to reduce objects created?
----------------------------------------------------------------
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 #863: Make
ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
Posted by GitBox <gi...@apache.org>.
narendly merged pull request #863: Make ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
URL: https://github.com/apache/helix/pull/863
----------------------------------------------------------------
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] lei-xia commented on a change in pull request #863: Make
ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
Posted by GitBox <gi...@apache.org>.
lei-xia commented on a change in pull request #863: Make ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
URL: https://github.com/apache/helix/pull/863#discussion_r389469451
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
##########
@@ -842,4 +894,116 @@ public void close() {
_zkClient.close();
}
}
+
+ public static class Builder {
Review comment:
1) This Builder code is very similar across all Helix APIs, I do recommend us to find some way to consolidate them.
2) For existing use case (who do not care about realm, or who is not going to shard to multiple ZK), do they need to change anything in their 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 a change in pull request #863: Make
ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #863: Make ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
URL: https://github.com/apache/helix/pull/863#discussion_r390727534
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
##########
@@ -90,50 +104,62 @@ public ZkCacheBaseDataAccessor(ZkBaseDataAccessor<T> baseAccessor, String chroot
_wtCachePaths = wtCachePaths;
_zkCachePaths = zkCachePaths;
- // TODO: need to make sure no overlap between wtCachePaths and zkCachePaths
- // TreeMap key is ordered by key string length, so more general (i.e. short) prefix
- // comes first
- _cacheMap = new TreeMap<>(new Comparator<String>() {
- @Override
- public int compare(String o1, String o2) {
- int len1 = o1.split("/").length;
- int len2 = o2.split("/").length;
- return len1 - len2;
- }
- });
-
start();
}
+ @Deprecated
public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String chrootPath,
List<String> wtCachePaths, List<String> zkCachePaths) {
this(zkAddress, serializer, chrootPath, wtCachePaths, zkCachePaths, null, null,
ZkBaseDataAccessor.ZkClientType.SHARED);
}
+ @Deprecated
public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String chrootPath,
List<String> wtCachePaths, List<String> zkCachePaths, String monitorType, String monitorkey) {
this(zkAddress, serializer, chrootPath, wtCachePaths, zkCachePaths, monitorType, monitorkey,
ZkBaseDataAccessor.ZkClientType.SHARED);
}
+ @Deprecated
public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String chrootPath,
List<String> wtCachePaths, List<String> zkCachePaths, String monitorType, String monitorkey,
ZkBaseDataAccessor.ZkClientType zkClientType) {
- HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig();
- clientConfig.setZkSerializer(serializer).setMonitorType(monitorType).setMonitorKey(monitorkey);
- switch (zkClientType) {
- case DEDICATED:
- _zkClient = DedicatedZkClientFactory.getInstance().buildZkClient(
- new HelixZkClient.ZkConnectionConfig(zkAddress),
- new HelixZkClient.ZkClientConfig().setZkSerializer(serializer));
- break;
- case SHARED:
- default:
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress), clientConfig);
- }
- _zkClient.waitUntilConnected(HelixZkClient.DEFAULT_CONNECTION_TIMEOUT, TimeUnit.MILLISECONDS);
+
+ // If the multi ZK config is enabled, use multi-realm mode with FederatedZkClient
+ if (Boolean.parseBoolean(System.getProperty(SystemPropertyKeys.MULTI_ZK_ENABLED))) {
+ try {
+ RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder connectionConfigBuilder =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder();
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig();
+ clientConfig.setZkSerializer(serializer).setMonitorType(monitorType)
+ .setMonitorKey(monitorkey);
+ // Use a federated zk client
+ _zkClient = new FederatedZkClient(connectionConfigBuilder.build(), clientConfig);
+ } catch (IOException | InvalidRoutingDataException | IllegalStateException e) {
+ // Note: IllegalStateException is for HttpRoutingDataReader if MSDS endpoint cannot be
+ // found
+ throw new HelixException("Failed to create ZkCacheBaseDataAccessor!", e);
+ }
+ } else {
+ HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig();
+ clientConfig.setZkSerializer(serializer).setMonitorType(monitorType)
+ .setMonitorKey(monitorkey);
+ switch (zkClientType) {
+ case DEDICATED:
+ _zkClient = DedicatedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
+ new HelixZkClient.ZkClientConfig().setZkSerializer(serializer));
+ break;
+ case SHARED:
+ default:
+ _zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress), clientConfig);
+ }
+ _zkClient.waitUntilConnected(HelixZkClient.DEFAULT_CONNECTION_TIMEOUT, TimeUnit.MILLISECONDS);
Review comment:
Note: current FederatedZkClient doesn't support `waitUntilConnected()` because of multiple raw zkClients. We shall move `waitUntilConnected` to only single-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 #863: Make
ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #863: Make ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
URL: https://github.com/apache/helix/pull/863#discussion_r389029493
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
##########
@@ -58,7 +61,15 @@
protected ZkCallbackCache<T> _zkCache;
final ZkBaseDataAccessor<T> _baseAccessor;
- final Map<String, Cache<T>> _cacheMap;
+
+ // TODO: need to make sure no overlap between wtCachePaths and zkCachePaths
+ // TreeMap key is ordered by key string length, so more general (i.e. short) prefix
+ // comes first
+ final Map<String, Cache<T>> _cacheMap = new TreeMap<>((o1, o2) -> {
+ int len1 = o1.split("/").length;
+ int len2 = o2.split("/").length;
Review comment:
Realized this is original code. We could resolve this thread.
----------------------------------------------------------------
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 #863: Make
ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #863: Make ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
URL: https://github.com/apache/helix/pull/863#discussion_r389847926
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
##########
@@ -146,17 +161,55 @@ public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String
_wtCachePaths = wtCachePaths;
_zkCachePaths = zkCachePaths;
- // TODO: need to make sure no overlap between wtCachePaths and zkCachePaths
- // TreeMap key is ordered by key string length, so more general (i.e. short) prefix
- // comes first
- _cacheMap = new TreeMap<>(new Comparator<String>() {
- @Override
- public int compare(String o1, String o2) {
- int len1 = o1.split("/").length;
- int len2 = o2.split("/").length;
- return len1 - len2;
- }
- });
+ start();
+ }
+
+ /**
+ * Constructor using a Builder that allows users to set connection and client configs.
+ * @param builder
+ */
+ private ZkCacheBaseDataAccessor(Builder builder) {
+ _chrootPath = builder._chrootPath;
+ _wtCachePaths = builder._wtCachePaths;
+ _zkCachePaths = builder._zkCachePaths;
+
+ RealmAwareZkClient zkClient;
+ switch (builder._realmMode) {
+ case MULTI_REALM:
+ try {
+ zkClient = new FederatedZkClient(builder._realmAwareZkConnectionConfig,
+ builder._realmAwareZkClientConfig);
+ break; // Must break out of the switch statement here
+ } catch (IOException | InvalidRoutingDataException | IllegalStateException e) {
Review comment:
It's already done in the builder?
----------------------------------------------------------------
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 #863: Make
ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #863: Make ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
URL: https://github.com/apache/helix/pull/863#discussion_r391372677
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
##########
@@ -90,50 +104,62 @@ public ZkCacheBaseDataAccessor(ZkBaseDataAccessor<T> baseAccessor, String chroot
_wtCachePaths = wtCachePaths;
_zkCachePaths = zkCachePaths;
- // TODO: need to make sure no overlap between wtCachePaths and zkCachePaths
- // TreeMap key is ordered by key string length, so more general (i.e. short) prefix
- // comes first
- _cacheMap = new TreeMap<>(new Comparator<String>() {
- @Override
- public int compare(String o1, String o2) {
- int len1 = o1.split("/").length;
- int len2 = o2.split("/").length;
- return len1 - len2;
- }
- });
-
start();
}
+ @Deprecated
public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String chrootPath,
List<String> wtCachePaths, List<String> zkCachePaths) {
this(zkAddress, serializer, chrootPath, wtCachePaths, zkCachePaths, null, null,
ZkBaseDataAccessor.ZkClientType.SHARED);
}
+ @Deprecated
public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String chrootPath,
List<String> wtCachePaths, List<String> zkCachePaths, String monitorType, String monitorkey) {
this(zkAddress, serializer, chrootPath, wtCachePaths, zkCachePaths, monitorType, monitorkey,
ZkBaseDataAccessor.ZkClientType.SHARED);
}
+ @Deprecated
public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String chrootPath,
List<String> wtCachePaths, List<String> zkCachePaths, String monitorType, String monitorkey,
ZkBaseDataAccessor.ZkClientType zkClientType) {
- HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig();
- clientConfig.setZkSerializer(serializer).setMonitorType(monitorType).setMonitorKey(monitorkey);
- switch (zkClientType) {
- case DEDICATED:
- _zkClient = DedicatedZkClientFactory.getInstance().buildZkClient(
- new HelixZkClient.ZkConnectionConfig(zkAddress),
- new HelixZkClient.ZkClientConfig().setZkSerializer(serializer));
- break;
- case SHARED:
- default:
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress), clientConfig);
- }
- _zkClient.waitUntilConnected(HelixZkClient.DEFAULT_CONNECTION_TIMEOUT, TimeUnit.MILLISECONDS);
+
+ // If the multi ZK config is enabled, use multi-realm mode with FederatedZkClient
+ if (Boolean.parseBoolean(System.getProperty(SystemPropertyKeys.MULTI_ZK_ENABLED))) {
+ try {
+ RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder connectionConfigBuilder =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder();
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig();
+ clientConfig.setZkSerializer(serializer).setMonitorType(monitorType)
+ .setMonitorKey(monitorkey);
+ // Use a federated zk client
+ _zkClient = new FederatedZkClient(connectionConfigBuilder.build(), clientConfig);
+ } catch (IOException | InvalidRoutingDataException | IllegalStateException e) {
+ // Note: IllegalStateException is for HttpRoutingDataReader if MSDS endpoint cannot be
+ // found
+ throw new HelixException("Failed to create ZkCacheBaseDataAccessor!", e);
+ }
+ } else {
+ HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig();
+ clientConfig.setZkSerializer(serializer).setMonitorType(monitorType)
+ .setMonitorKey(monitorkey);
+ switch (zkClientType) {
+ case DEDICATED:
+ _zkClient = DedicatedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
+ new HelixZkClient.ZkClientConfig().setZkSerializer(serializer));
+ break;
+ case SHARED:
+ default:
+ _zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress), clientConfig);
+ }
+ _zkClient.waitUntilConnected(HelixZkClient.DEFAULT_CONNECTION_TIMEOUT, TimeUnit.MILLISECONDS);
Review comment:
@pkuwm Could you look again? This is in the else block where we do not create FederatedZkClient...
----------------------------------------------------------------
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 #863: Make
ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #863: Make ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
URL: https://github.com/apache/helix/pull/863#discussion_r391372677
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
##########
@@ -90,50 +104,62 @@ public ZkCacheBaseDataAccessor(ZkBaseDataAccessor<T> baseAccessor, String chroot
_wtCachePaths = wtCachePaths;
_zkCachePaths = zkCachePaths;
- // TODO: need to make sure no overlap between wtCachePaths and zkCachePaths
- // TreeMap key is ordered by key string length, so more general (i.e. short) prefix
- // comes first
- _cacheMap = new TreeMap<>(new Comparator<String>() {
- @Override
- public int compare(String o1, String o2) {
- int len1 = o1.split("/").length;
- int len2 = o2.split("/").length;
- return len1 - len2;
- }
- });
-
start();
}
+ @Deprecated
public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String chrootPath,
List<String> wtCachePaths, List<String> zkCachePaths) {
this(zkAddress, serializer, chrootPath, wtCachePaths, zkCachePaths, null, null,
ZkBaseDataAccessor.ZkClientType.SHARED);
}
+ @Deprecated
public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String chrootPath,
List<String> wtCachePaths, List<String> zkCachePaths, String monitorType, String monitorkey) {
this(zkAddress, serializer, chrootPath, wtCachePaths, zkCachePaths, monitorType, monitorkey,
ZkBaseDataAccessor.ZkClientType.SHARED);
}
+ @Deprecated
public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String chrootPath,
List<String> wtCachePaths, List<String> zkCachePaths, String monitorType, String monitorkey,
ZkBaseDataAccessor.ZkClientType zkClientType) {
- HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig();
- clientConfig.setZkSerializer(serializer).setMonitorType(monitorType).setMonitorKey(monitorkey);
- switch (zkClientType) {
- case DEDICATED:
- _zkClient = DedicatedZkClientFactory.getInstance().buildZkClient(
- new HelixZkClient.ZkConnectionConfig(zkAddress),
- new HelixZkClient.ZkClientConfig().setZkSerializer(serializer));
- break;
- case SHARED:
- default:
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress), clientConfig);
- }
- _zkClient.waitUntilConnected(HelixZkClient.DEFAULT_CONNECTION_TIMEOUT, TimeUnit.MILLISECONDS);
+
+ // If the multi ZK config is enabled, use multi-realm mode with FederatedZkClient
+ if (Boolean.parseBoolean(System.getProperty(SystemPropertyKeys.MULTI_ZK_ENABLED))) {
+ try {
+ RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder connectionConfigBuilder =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder();
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig();
+ clientConfig.setZkSerializer(serializer).setMonitorType(monitorType)
+ .setMonitorKey(monitorkey);
+ // Use a federated zk client
+ _zkClient = new FederatedZkClient(connectionConfigBuilder.build(), clientConfig);
+ } catch (IOException | InvalidRoutingDataException | IllegalStateException e) {
+ // Note: IllegalStateException is for HttpRoutingDataReader if MSDS endpoint cannot be
+ // found
+ throw new HelixException("Failed to create ZkCacheBaseDataAccessor!", e);
+ }
+ } else {
+ HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig();
+ clientConfig.setZkSerializer(serializer).setMonitorType(monitorType)
+ .setMonitorKey(monitorkey);
+ switch (zkClientType) {
+ case DEDICATED:
+ _zkClient = DedicatedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
+ new HelixZkClient.ZkClientConfig().setZkSerializer(serializer));
+ break;
+ case SHARED:
+ default:
+ _zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress), clientConfig);
+ }
+ _zkClient.waitUntilConnected(HelixZkClient.DEFAULT_CONNECTION_TIMEOUT, TimeUnit.MILLISECONDS);
Review comment:
@pkuwm Could you look a little more closely? This is in the else block where we do not create FederatedZkClient...
----------------------------------------------------------------
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 #863: Make
ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on issue #863: Make ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware
URL: https://github.com/apache/helix/pull/863#issuecomment-597978393
This PR is ready to be merged, approved by @dasahcc
This commit makes both ZkCacheBaseDataAccessor and ZkHelixPropertyStore realm-aware by choosing the appropriate realm-aware ZkClients in the constructor. Also, we add a Builder here to give users options to set Connection config and Client config.
Note that ZkHelixPropertyStore extends CacheBaseDataAccessor so there is no change needed.
----------------------------------------------------------------
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