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/03 01:40:04 UTC
[GitHub] [helix] pkuwm opened a new pull request #846: [WIP] Make HelixAdmin
realm aware
pkuwm opened a new pull request #846: [WIP] Make HelixAdmin realm aware
URL: https://github.com/apache/helix/pull/846
This is a draft. The work depends on #838
----------------------------------------------------------------
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 #846: [WIP]
Make ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: [WIP] Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r387496499
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -89,33 +91,55 @@
public class ZKHelixAdmin implements HelixAdmin {
+ private static final Logger LOG = LoggerFactory.getLogger(ZKHelixAdmin.class);
+
+
public static final String CONNECTION_TIMEOUT = "helixAdmin.timeOutInSec";
private static final String MAINTENANCE_ZNODE_ID = "maintenance";
private static final int DEFAULT_SUPERCLUSTER_REPLICA = 3;
- private final HelixZkClient _zkClient;
+ private final RealmAwareZkClient _zkClient;
private final ConfigAccessor _configAccessor;
// true if ZKHelixAdmin was instantiated with a HelixZkClient, false otherwise
// This is used for close() to determine how ZKHelixAdmin should close the underlying ZkClient
private final boolean _usesExternalZkClient;
private static Logger logger = LoggerFactory.getLogger(ZKHelixAdmin.class);
- @Deprecated
- public ZKHelixAdmin(HelixZkClient zkClient) {
+ public ZKHelixAdmin() {
+ _zkClient = new FederatedZkClient();
+ _configAccessor = new ConfigAccessor(_zkClient);
+ _usesExternalZkClient = false;
+ }
+
+ public ZKHelixAdmin(RealmAwareZkClient zkClient) {
_zkClient = zkClient;
_configAccessor = new ConfigAccessor(zkClient);
_usesExternalZkClient = true;
}
public ZKHelixAdmin(String zkAddress) {
int timeOutInSec = Integer.parseInt(System.getProperty(CONNECTION_TIMEOUT, "30"));
- HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig();
- clientConfig.setZkSerializer(new ZNRecordSerializer())
- .setConnectInitTimeout(timeOutInSec * 1000);
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress), clientConfig);
- _zkClient.waitUntilConnected(timeOutInSec, TimeUnit.SECONDS);
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
Review comment:
I just realized I forgot to add this logic for ConfigAccessor. Let me open another PR :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r388554676
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -89,33 +91,77 @@
public class ZKHelixAdmin implements HelixAdmin {
+ private static final Logger LOG = LoggerFactory.getLogger(ZKHelixAdmin.class);
+
+
public static final String CONNECTION_TIMEOUT = "helixAdmin.timeOutInSec";
private static final String MAINTENANCE_ZNODE_ID = "maintenance";
private static final int DEFAULT_SUPERCLUSTER_REPLICA = 3;
- private final HelixZkClient _zkClient;
+ private final RealmAwareZkClient _zkClient;
private final ConfigAccessor _configAccessor;
- // true if ZKHelixAdmin was instantiated with a HelixZkClient, false otherwise
+ // true if ZKHelixAdmin was instantiated with a RealmAwareZkClient, false otherwise
// This is used for close() to determine how ZKHelixAdmin should close the underlying ZkClient
private final boolean _usesExternalZkClient;
private static Logger logger = LoggerFactory.getLogger(ZKHelixAdmin.class);
+ /**
+ * @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 ZKHelixAdmin(HelixZkClient zkClient) {
+ public ZKHelixAdmin(RealmAwareZkClient zkClient) {
_zkClient = zkClient;
_configAccessor = new ConfigAccessor(zkClient);
_usesExternalZkClient = true;
}
public ZKHelixAdmin(String zkAddress) {
int timeOutInSec = Integer.parseInt(System.getProperty(CONNECTION_TIMEOUT, "30"));
- HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig();
- clientConfig.setZkSerializer(new ZNRecordSerializer())
- .setConnectInitTimeout(timeOutInSec * 1000);
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress), clientConfig);
- _zkClient.waitUntilConnected(timeOutInSec, TimeUnit.SECONDS);
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig()
+ .setConnectInitTimeout(timeOutInSec * 1000L)
+ .setZkSerializer(new ZNRecordSerializer());
+
+ RealmAwareZkClient zkClient;
+ try {
+ zkClient = new FederatedZkClient(
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(), clientConfig);
+ } catch (IllegalStateException | IOException | InvalidRoutingDataException e) {
+ LOG.info("Not able to connect on multi-realm mode. "
+ + "Connecting on single-realm mode to ZK: {}", zkAddress);
+
+ zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
+ clientConfig.createHelixZkClientConfig());
+ zkClient.waitUntilConnected(timeOutInSec, TimeUnit.SECONDS);
+ }
+
+ _zkClient = zkClient;
+ _configAccessor = new ConfigAccessor(_zkClient);
+ _usesExternalZkClient = false;
+ }
+
+ private ZKHelixAdmin(Builder builder) throws IOException, InvalidRoutingDataException {
Review comment:
I followed `ConfigAccessor` to make it consistent. If `ConfigAccessor ` is not a special case, we may also need to change it?
```
private ConfigAccessor(Builder builder) throws IOException, InvalidRoutingDataException {
```
----------------------------------------------------------------
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 #846: [WIP]
Make ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: [WIP] Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r387498324
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1285,4 +1264,87 @@ public void handleSessionEstablishmentError(Throwable error) throws Exception {
public Long getSessionStartTime() {
return _sessionStartTime;
}
+
+ private RealmAwareZkClient buildRealmAwareZkClient() {
+ final String shardingKey = buildShardingKey();
+ PathBasedZkSerializer zkSerializer =
+ ChainedPathZkSerializer.builder(new ZNRecordSerializer()).build();
+
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
+ .setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
+ .setZkRealmShardingKey(shardingKey)
+ .setSessionTimeout(_sessionTimeout).build();
+
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig();
+
+ clientConfig.setZkSerializer(zkSerializer)
+ .setConnectInitTimeout(_connectionInitTimeout)
+ .setMonitorType(_instanceType.name())
+ .setMonitorKey(_clusterName)
+ .setMonitorInstanceName(_instanceName)
+ .setMonitorRootPathOnly(isMonitorRootPathOnly());
+
+ RealmAwareZkClient newClient;
+ if (_instanceType == InstanceType.ADMINISTRATOR) {
+ newClient = buildSharedZkClient(connectionConfig, clientConfig);
+ } else {
+ newClient = buildDedicatedZkClient(connectionConfig, clientConfig);
+ }
+
+ return newClient;
+ }
+
+ private RealmAwareZkClient buildSharedZkClient(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig,
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig) {
+ RealmAwareZkClient zkClient;
+ // TODO: change exception type
+ try {
+ zkClient =
+ SharedZkClientFactory.getInstance().buildZkClient(connectionConfig, clientConfig);
+ } catch (IllegalArgumentException e) {
+ LOG.warn("Not able to build realm-aware shared ZK client for sharding key: {}, caused by: {}"
+ + " Fallback to HelixZkClient.", connectionConfig.getZkRealmShardingKey(),
+ e.getMessage());
+
+ // Fallback to HelixZkClient
+ HelixZkClient.ZkConnectionConfig helixZkConnectionConfig =
+ new HelixZkClient.ZkConnectionConfig(_zkAddress);
+ helixZkConnectionConfig.setSessionTimeout(connectionConfig.getSessionTimeout());
+ zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(helixZkConnectionConfig, clientConfig.createHelixZkClientConfig());
+ }
+
+ return zkClient;
+ }
+
+ private RealmAwareZkClient buildDedicatedZkClient(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig,
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig) {
+ RealmAwareZkClient zkClient;
+ // TODO: change exception type
+ try {
+ zkClient =
+ DedicatedZkClientFactory.getInstance().buildZkClient(connectionConfig, clientConfig);
+ } catch (IllegalArgumentException e) {
+ LOG.warn("Not able to build realm-aware Dedicated ZK client for sharding key: {}, caused by: {}"
+ + " Fallback to HelixZkClient.", connectionConfig.getZkRealmShardingKey(),
+ e.getMessage());
+
+ // Fallback to HelixZkClient
+ HelixZkClient.ZkConnectionConfig helixZkConnectionConfig =
+ new HelixZkClient.ZkConnectionConfig(_zkAddress);
+ helixZkConnectionConfig.setSessionTimeout(connectionConfig.getSessionTimeout());
+ zkClient = DedicatedZkClientFactory.getInstance()
+ .buildZkClient(helixZkConnectionConfig, clientConfig.createHelixZkClientConfig());
+ }
Review comment:
Do you think we could refactor these two methods to reduce duplicate logic? You could take in the factory in the parameter and switch back and forth between dedicated and shared that way.
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r388514219
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -89,33 +91,77 @@
public class ZKHelixAdmin implements HelixAdmin {
+ private static final Logger LOG = LoggerFactory.getLogger(ZKHelixAdmin.class);
+
+
public static final String CONNECTION_TIMEOUT = "helixAdmin.timeOutInSec";
private static final String MAINTENANCE_ZNODE_ID = "maintenance";
private static final int DEFAULT_SUPERCLUSTER_REPLICA = 3;
- private final HelixZkClient _zkClient;
+ private final RealmAwareZkClient _zkClient;
private final ConfigAccessor _configAccessor;
- // true if ZKHelixAdmin was instantiated with a HelixZkClient, false otherwise
+ // true if ZKHelixAdmin was instantiated with a RealmAwareZkClient, false otherwise
// This is used for close() to determine how ZKHelixAdmin should close the underlying ZkClient
private final boolean _usesExternalZkClient;
private static Logger logger = LoggerFactory.getLogger(ZKHelixAdmin.class);
+ /**
+ * @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 ZKHelixAdmin(HelixZkClient zkClient) {
+ public ZKHelixAdmin(RealmAwareZkClient zkClient) {
_zkClient = zkClient;
_configAccessor = new ConfigAccessor(zkClient);
_usesExternalZkClient = true;
}
public ZKHelixAdmin(String zkAddress) {
int timeOutInSec = Integer.parseInt(System.getProperty(CONNECTION_TIMEOUT, "30"));
- HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig();
- clientConfig.setZkSerializer(new ZNRecordSerializer())
- .setConnectInitTimeout(timeOutInSec * 1000);
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress), clientConfig);
- _zkClient.waitUntilConnected(timeOutInSec, TimeUnit.SECONDS);
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig()
+ .setConnectInitTimeout(timeOutInSec * 1000L)
+ .setZkSerializer(new ZNRecordSerializer());
+
+ RealmAwareZkClient zkClient;
+ try {
+ zkClient = new FederatedZkClient(
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(), clientConfig);
+ } catch (IllegalStateException | IOException | InvalidRoutingDataException e) {
+ LOG.info("Not able to connect on multi-realm mode. "
+ + "Connecting on single-realm mode to ZK: {}", zkAddress);
+
+ zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
+ clientConfig.createHelixZkClientConfig());
+ zkClient.waitUntilConnected(timeOutInSec, TimeUnit.SECONDS);
+ }
+
+ _zkClient = zkClient;
+ _configAccessor = new ConfigAccessor(_zkClient);
+ _usesExternalZkClient = false;
+ }
+
+ private ZKHelixAdmin(Builder builder) throws IOException, InvalidRoutingDataException {
+ switch (builder.realmMode) {
+ case MULTI_REALM:
+ _zkClient = new FederatedZkClient(builder.realmAwareZkConnectionConfig,
+ builder.realmAwareZkClientConfig);
Review comment:
Example: https://github.com/apache/helix/pull/863/files#diff-259ce65919d1337b08cb736839d755feR178-R192
What are your thoughts?
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r388754151
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -1799,4 +1856,69 @@ private boolean validateWeightForResourceConfig(ClusterConfig clusterConfig,
clusterConfig));
return true;
}
+
+ public static class Builder {
+ private String zkAddress;
+ private RealmAwareZkClient.RealmMode realmMode;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig;
+
+ public Builder() {
+ }
+
+ public ZKHelixAdmin.Builder setZkAddress(String zkAddress) {
+ this.zkAddress = zkAddress;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ this.realmMode = realmMode;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return this;
+ }
+
+ public ZKHelixAdmin build() throws Exception {
+ validate();
+ return new ZKHelixAdmin(this);
+ }
+
+ /**
+ * Validate the given parameters before creating an instance of ConfigAccessor.
Review comment:
ConfigAccessor -> ZkHelixAdmin?
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r391402895
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1285,4 +1266,60 @@ public void handleSessionEstablishmentError(Throwable error) throws Exception {
public Long getSessionStartTime() {
return _sessionStartTime;
}
+
+ private RealmAwareZkClient createSingleRealmZkClient() {
Review comment:
- Rename the method to `prepareAndGetZkClient()`
- Javadoc:
Prepares connection config and client config based on the internal parameters given to HelixManager in order to create a ZkClient instance to use. Note that a shared ZkClient instance will be created if connecting as an ADMINISTRATOR to minimize the cost of creating ZkConnections.
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r388683543
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -1799,4 +1845,71 @@ private boolean validateWeightForResourceConfig(ClusterConfig clusterConfig,
clusterConfig));
return true;
}
+
+ public static class Builder {
+ private String zkAddress;
+ private RealmAwareZkClient.RealmMode realmMode;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig;
+
+ public Builder() {
+ }
+
+ public ZKHelixAdmin.Builder setZkAddress(String zkAddress) {
+ this.zkAddress = zkAddress;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ this.realmMode = realmMode;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return this;
+ }
+
+ public ZKHelixAdmin build() throws Exception {
+ validate();
+ return new ZKHelixAdmin(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());
Review comment:
Done. We may want to make `ConfigAccessor` consistent with this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r391402054
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1285,4 +1266,60 @@ public void handleSessionEstablishmentError(Throwable error) throws Exception {
public Long getSessionStartTime() {
return _sessionStartTime;
}
+
+ private RealmAwareZkClient createSingleRealmZkClient() {
+ final String shardingKey = buildShardingKey();
+ PathBasedZkSerializer zkSerializer =
+ ChainedPathZkSerializer.builder(new ZNRecordSerializer()).build();
+
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
+ .setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
+ .setZkRealmShardingKey(shardingKey)
+ .setSessionTimeout(_sessionTimeout).build();
+
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig();
+
+ clientConfig.setZkSerializer(zkSerializer)
+ .setConnectInitTimeout(_connectionInitTimeout)
+ .setMonitorType(_instanceType.name())
+ .setMonitorKey(_clusterName)
+ .setMonitorInstanceName(_instanceName)
+ .setMonitorRootPathOnly(isMonitorRootPathOnly());
+
+ if (_instanceType == InstanceType.ADMINISTRATOR) {
+ return buildSelectiveZkClient(SharedZkClientFactory.getInstance(), connectionConfig,
+ clientConfig);
+ }
+
+ return buildSelectiveZkClient(DedicatedZkClientFactory.getInstance(), connectionConfig,
+ clientConfig);
+ }
+
+ private RealmAwareZkClient buildSelectiveZkClient(HelixZkClientFactory zkClientFactory,
Review comment:
My suggestion is
JavaDoc:
Resolves what type of ZkClient this HelixManager should use based on whether MULTI_ZK_ENABLED System config is set or not. Two types of ZkClients are available: 1) If MULTI_ZK_ENABLED is set to true, we create a dedicated RealmAwareZkClient that provides full ZkClient functionalities and connects to the correct ZK by querying MetadataStoreDirectoryService. 2) Otherwise, we create a dedicated HelixZkClient which plainly connects to the ZK address given.
- Rename the method to `resolveZkClient()`
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r388500352
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1285,4 +1264,87 @@ public void handleSessionEstablishmentError(Throwable error) throws Exception {
public Long getSessionStartTime() {
return _sessionStartTime;
}
+
+ private RealmAwareZkClient buildRealmAwareZkClient() {
+ final String shardingKey = buildShardingKey();
+ PathBasedZkSerializer zkSerializer =
+ ChainedPathZkSerializer.builder(new ZNRecordSerializer()).build();
+
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
+ .setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
+ .setZkRealmShardingKey(shardingKey)
+ .setSessionTimeout(_sessionTimeout).build();
+
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig();
+
+ clientConfig.setZkSerializer(zkSerializer)
+ .setConnectInitTimeout(_connectionInitTimeout)
+ .setMonitorType(_instanceType.name())
+ .setMonitorKey(_clusterName)
+ .setMonitorInstanceName(_instanceName)
+ .setMonitorRootPathOnly(isMonitorRootPathOnly());
+
+ RealmAwareZkClient newClient;
+ if (_instanceType == InstanceType.ADMINISTRATOR) {
+ newClient = buildSharedZkClient(connectionConfig, clientConfig);
+ } else {
+ newClient = buildDedicatedZkClient(connectionConfig, clientConfig);
+ }
+
+ return newClient;
+ }
+
+ private RealmAwareZkClient buildSharedZkClient(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig,
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig) {
+ RealmAwareZkClient zkClient;
+ // TODO: change exception type
+ try {
+ zkClient =
+ SharedZkClientFactory.getInstance().buildZkClient(connectionConfig, clientConfig);
+ } catch (IllegalArgumentException e) {
+ LOG.warn("Not able to build realm-aware shared ZK client for sharding key: {}, caused by: {}"
+ + " Fallback to HelixZkClient.", connectionConfig.getZkRealmShardingKey(),
+ e.getMessage());
+
+ // Fallback to HelixZkClient
+ HelixZkClient.ZkConnectionConfig helixZkConnectionConfig =
+ new HelixZkClient.ZkConnectionConfig(_zkAddress);
+ helixZkConnectionConfig.setSessionTimeout(connectionConfig.getSessionTimeout());
+ zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(helixZkConnectionConfig, clientConfig.createHelixZkClientConfig());
+ }
+
+ return zkClient;
+ }
+
+ private RealmAwareZkClient buildDedicatedZkClient(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig,
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig) {
+ RealmAwareZkClient zkClient;
+ // TODO: change exception type
+ try {
+ zkClient =
+ DedicatedZkClientFactory.getInstance().buildZkClient(connectionConfig, clientConfig);
+ } catch (IllegalArgumentException e) {
+ LOG.warn("Not able to build realm-aware Dedicated ZK client for sharding key: {}, caused by: {}"
+ + " Fallback to HelixZkClient.", connectionConfig.getZkRealmShardingKey(),
+ e.getMessage());
+
+ // Fallback to HelixZkClient
+ HelixZkClient.ZkConnectionConfig helixZkConnectionConfig =
+ new HelixZkClient.ZkConnectionConfig(_zkAddress);
+ helixZkConnectionConfig.setSessionTimeout(connectionConfig.getSessionTimeout());
+ zkClient = DedicatedZkClientFactory.getInstance()
+ .buildZkClient(helixZkConnectionConfig, clientConfig.createHelixZkClientConfig());
+ }
Review comment:
I tried. We have 2 options:
1. As you said, we can pass in the factory as a parameter. But we have to make `HelixZkClientFactory` public. Shall we do this?
`public abstract class HelixZkClientFactory implements RealmAwareZkClientFactory`
2. It is the implemented as I do. Keep`HelixZkClientFactory` as it is. I prefer this considering that we don't want HelixZkClientFactory public.
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r390792533
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1285,4 +1266,63 @@ public void handleSessionEstablishmentError(Throwable error) throws Exception {
public Long getSessionStartTime() {
return _sessionStartTime;
}
+
+ private RealmAwareZkClient buildRealmAwareZkClient() {
+ final String shardingKey = buildShardingKey();
+ PathBasedZkSerializer zkSerializer =
+ ChainedPathZkSerializer.builder(new ZNRecordSerializer()).build();
+
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
+ .setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
+ .setZkRealmShardingKey(shardingKey)
+ .setSessionTimeout(_sessionTimeout).build();
+
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig();
+
+ clientConfig.setZkSerializer(zkSerializer)
+ .setConnectInitTimeout(_connectionInitTimeout)
+ .setMonitorType(_instanceType.name())
+ .setMonitorKey(_clusterName)
+ .setMonitorInstanceName(_instanceName)
+ .setMonitorRootPathOnly(isMonitorRootPathOnly());
+
+ RealmAwareZkClient newClient;
+ if (_instanceType == InstanceType.ADMINISTRATOR) {
+ newClient = buildHelixZkClient(SharedZkClientFactory.getInstance(), connectionConfig,
+ clientConfig);
+ } else {
+ newClient = buildHelixZkClient(DedicatedZkClientFactory.getInstance(), connectionConfig,
+ clientConfig);
+ }
+
+ return newClient;
+ }
+
+ private RealmAwareZkClient buildHelixZkClient(HelixZkClientFactory zkClientFactory,
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig,
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig) {
+ if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED)) {
+ try {
+ // Create realm-aware ZkClient.
+ return zkClientFactory.buildZkClient(connectionConfig, clientConfig);
+ } catch (IllegalArgumentException | IOException | InvalidRoutingDataException e) {
+ throw new HelixException("Not able to connect on realm-aware mode for sharding key: "
+ + connectionConfig.getZkRealmShardingKey(), e);
+ }
+ }
+
+ // If multi-zk mode is not enabled, create HelixZkClient with the provided zk address.
+ HelixZkClient.ZkClientConfig helixZkClientConfig = clientConfig.createHelixZkClientConfig();
+ HelixZkClient.ZkConnectionConfig helixZkConnectionConfig =
+ new HelixZkClient.ZkConnectionConfig(_zkAddress)
+ .setSessionTimeout(connectionConfig.getSessionTimeout());
+
+ return zkClientFactory.buildZkClient(helixZkConnectionConfig, helixZkClientConfig);
+ }
Review comment:
These method names are very confusing. You're creating HelixZkClient in buildRealmAwareZkClient() and creating RealmAwareZkClient in buildHelixZkClient()? Could we try to improve the naming for these methods so that it's easier to read? I think this could be refactored for readability. Thoughts?
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r388514880
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -1799,4 +1845,71 @@ private boolean validateWeightForResourceConfig(ClusterConfig clusterConfig,
clusterConfig));
return true;
}
+
+ public static class Builder {
+ private String zkAddress;
+ private RealmAwareZkClient.RealmMode realmMode;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig;
+
+ public Builder() {
+ }
+
+ public ZKHelixAdmin.Builder setZkAddress(String zkAddress) {
+ this.zkAddress = zkAddress;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ this.realmMode = realmMode;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return this;
+ }
+
+ public ZKHelixAdmin build() throws Exception {
+ validate();
+ return new ZKHelixAdmin(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());
Review comment:
This could actually be simplified to
```
// Resolve RealmAwareZkClientConfig
if (_realmAwareZkClientConfig == null) {
_realmAwareZkClientConfig = new RealmAwareZkClient.RealmAwareZkClientConfig();
}
```
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r389321437
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -1799,4 +1851,69 @@ private boolean validateWeightForResourceConfig(ClusterConfig clusterConfig,
clusterConfig));
return true;
}
+
+ public static class Builder {
+ private String zkAddress;
+ private RealmAwareZkClient.RealmMode realmMode;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig;
+
+ public Builder() {
+ }
+
+ public ZKHelixAdmin.Builder setZkAddress(String zkAddress) {
+ this.zkAddress = zkAddress;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ this.realmMode = realmMode;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return this;
+ }
+
+ public ZKHelixAdmin build() throws Exception {
+ validate();
+ return new ZKHelixAdmin(this);
+ }
+
+ /*
+ * Validates the given parameters before creating an instance of ZKHelixAdmin.
+ */
+ 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!");
Review comment:
This is not configAccessor... lets fix the log? :)
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r389321569
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -1799,4 +1851,69 @@ private boolean validateWeightForResourceConfig(ClusterConfig clusterConfig,
clusterConfig));
return true;
}
+
+ public static class Builder {
+ private String zkAddress;
+ private RealmAwareZkClient.RealmMode realmMode;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig;
+
+ public Builder() {
+ }
+
+ public ZKHelixAdmin.Builder setZkAddress(String zkAddress) {
+ this.zkAddress = zkAddress;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ this.realmMode = realmMode;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return this;
+ }
+
+ public ZKHelixAdmin build() throws Exception {
+ validate();
+ return new ZKHelixAdmin(this);
+ }
+
+ /*
+ * Validates the given parameters before creating an instance of ZKHelixAdmin.
+ */
+ private void validate() {
+ // Resolve RealmMode based on other parameters
+ boolean isZkAddressSet = zkAddress != null && !zkAddress.isEmpty();
+ if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM && !isZkAddressSet) {
Review comment:
Also we need to throw exception when zkaddr is set 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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r388500405
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1285,4 +1266,85 @@ public void handleSessionEstablishmentError(Throwable error) throws Exception {
public Long getSessionStartTime() {
return _sessionStartTime;
}
+
+ private RealmAwareZkClient buildRealmAwareZkClient() {
+ final String shardingKey = buildShardingKey();
+ PathBasedZkSerializer zkSerializer =
+ ChainedPathZkSerializer.builder(new ZNRecordSerializer()).build();
+
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
+ .setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
+ .setZkRealmShardingKey(shardingKey)
+ .setSessionTimeout(_sessionTimeout).build();
+
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig();
+
+ clientConfig.setZkSerializer(zkSerializer)
+ .setConnectInitTimeout(_connectionInitTimeout)
+ .setMonitorType(_instanceType.name())
+ .setMonitorKey(_clusterName)
+ .setMonitorInstanceName(_instanceName)
+ .setMonitorRootPathOnly(isMonitorRootPathOnly());
+
+ RealmAwareZkClient newClient;
+ if (_instanceType == InstanceType.ADMINISTRATOR) {
+ newClient = buildSharedZkClient(connectionConfig, clientConfig);
+ } else {
+ newClient = buildDedicatedZkClient(connectionConfig, clientConfig);
+ }
+
+ return newClient;
+ }
+
+ private RealmAwareZkClient buildSharedZkClient(
Review comment:
I tried. We have 2 options:
1. As you said, we can pass in the factory as a parameter. But we have to make `HelixZkClientFactory` public. Shall we do this?
`public abstract class HelixZkClientFactory implements RealmAwareZkClientFactory`
2. It is the implemented as I do. Keep`HelixZkClientFactory` as it is. I prefer this considering that we don't want HelixZkClientFactory public.
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r389319663
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -89,33 +91,88 @@
public class ZKHelixAdmin implements HelixAdmin {
+ private static final Logger LOG = LoggerFactory.getLogger(ZKHelixAdmin.class);
+
+
public static final String CONNECTION_TIMEOUT = "helixAdmin.timeOutInSec";
private static final String MAINTENANCE_ZNODE_ID = "maintenance";
private static final int DEFAULT_SUPERCLUSTER_REPLICA = 3;
- private final HelixZkClient _zkClient;
+ private final RealmAwareZkClient _zkClient;
private final ConfigAccessor _configAccessor;
- // true if ZKHelixAdmin was instantiated with a HelixZkClient, false otherwise
+ // true if ZKHelixAdmin was instantiated with a RealmAwareZkClient, false otherwise
// This is used for close() to determine how ZKHelixAdmin should close the underlying ZkClient
private final boolean _usesExternalZkClient;
private static Logger logger = LoggerFactory.getLogger(ZKHelixAdmin.class);
+ /**
+ * @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 ZKHelixAdmin(HelixZkClient zkClient) {
+ public ZKHelixAdmin(RealmAwareZkClient zkClient) {
_zkClient = zkClient;
_configAccessor = new ConfigAccessor(zkClient);
_usesExternalZkClient = true;
}
public ZKHelixAdmin(String zkAddress) {
int timeOutInSec = Integer.parseInt(System.getProperty(CONNECTION_TIMEOUT, "30"));
- HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig();
- clientConfig.setZkSerializer(new ZNRecordSerializer())
- .setConnectInitTimeout(timeOutInSec * 1000);
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress), clientConfig);
- _zkClient.waitUntilConnected(timeOutInSec, TimeUnit.SECONDS);
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig()
+ .setConnectInitTimeout(timeOutInSec * 1000L)
+ .setZkSerializer(new ZNRecordSerializer());
+
+ RealmAwareZkClient zkClient;
+ try {
+ zkClient = new FederatedZkClient(
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(), clientConfig);
+ } catch (IllegalStateException | IOException | InvalidRoutingDataException e) {
+ LOG.info("Not able to connect on multi-realm mode. "
+ + "Connecting on single-realm mode to ZK: {}", zkAddress);
+
+ zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
+ clientConfig.createHelixZkClientConfig());
+ zkClient.waitUntilConnected(timeOutInSec, TimeUnit.SECONDS);
+ }
+
+ _zkClient = zkClient;
+ _configAccessor = new ConfigAccessor(_zkClient);
+ _usesExternalZkClient = false;
+ }
+
+ private ZKHelixAdmin(Builder builder) {
+ RealmAwareZkClient zkClient;
+ switch (builder.realmMode) {
+ case MULTI_REALM:
+ try {
+ zkClient = new FederatedZkClient(builder.realmAwareZkConnectionConfig,
+ builder.realmAwareZkClientConfig);
+ 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);
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 issue #846: Make ZKHelixAdmin and
ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on issue #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#issuecomment-598052559
This PR is ready to be merged, approved by @narendly
```
To make Helix Java APIs realm-aware, we need to make both ZKHelixAdmin and ZKHelixManager realm-aware. This commit adds a Builder to set client config and connection config for building realm-aware ZkClients underneath.
```
----------------------------------------------------------------
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 #846: [WIP]
Make ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: [WIP] Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r387499467
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/FederatedZkClient.java
##########
@@ -79,7 +83,31 @@
private volatile boolean _isClosed;
private PathBasedZkSerializer _pathBasedZkSerializer;
+ public FederatedZkClient() {
+ this(new RealmAwareZkClient.RealmAwareZkClientConfig()
+ .setZkSerializer(new ZNRecordSerializer()));
+ }
+
// TODO: support capacity of ZkClient number in one FederatedZkClient and do garbage collection.
+ public FederatedZkClient(RealmAwareZkClient.RealmAwareZkClientConfig clientConfig) {
+ try {
Review comment:
FederatedZkClient should take in connectionConfig and clientConfig. I'll just assume this is a placeholder.
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r388513805
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -89,33 +91,77 @@
public class ZKHelixAdmin implements HelixAdmin {
+ private static final Logger LOG = LoggerFactory.getLogger(ZKHelixAdmin.class);
+
+
public static final String CONNECTION_TIMEOUT = "helixAdmin.timeOutInSec";
private static final String MAINTENANCE_ZNODE_ID = "maintenance";
private static final int DEFAULT_SUPERCLUSTER_REPLICA = 3;
- private final HelixZkClient _zkClient;
+ private final RealmAwareZkClient _zkClient;
private final ConfigAccessor _configAccessor;
- // true if ZKHelixAdmin was instantiated with a HelixZkClient, false otherwise
+ // true if ZKHelixAdmin was instantiated with a RealmAwareZkClient, false otherwise
// This is used for close() to determine how ZKHelixAdmin should close the underlying ZkClient
private final boolean _usesExternalZkClient;
private static Logger logger = LoggerFactory.getLogger(ZKHelixAdmin.class);
+ /**
+ * @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 ZKHelixAdmin(HelixZkClient zkClient) {
+ public ZKHelixAdmin(RealmAwareZkClient zkClient) {
_zkClient = zkClient;
_configAccessor = new ConfigAccessor(zkClient);
_usesExternalZkClient = true;
}
public ZKHelixAdmin(String zkAddress) {
int timeOutInSec = Integer.parseInt(System.getProperty(CONNECTION_TIMEOUT, "30"));
- HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig();
- clientConfig.setZkSerializer(new ZNRecordSerializer())
- .setConnectInitTimeout(timeOutInSec * 1000);
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress), clientConfig);
- _zkClient.waitUntilConnected(timeOutInSec, TimeUnit.SECONDS);
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig()
+ .setConnectInitTimeout(timeOutInSec * 1000L)
+ .setZkSerializer(new ZNRecordSerializer());
+
+ RealmAwareZkClient zkClient;
+ try {
+ zkClient = new FederatedZkClient(
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(), clientConfig);
+ } catch (IllegalStateException | IOException | InvalidRoutingDataException e) {
+ LOG.info("Not able to connect on multi-realm mode. "
+ + "Connecting on single-realm mode to ZK: {}", zkAddress);
+
+ zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
+ clientConfig.createHelixZkClientConfig());
+ zkClient.waitUntilConnected(timeOutInSec, TimeUnit.SECONDS);
+ }
+
+ _zkClient = zkClient;
+ _configAccessor = new ConfigAccessor(_zkClient);
+ _usesExternalZkClient = false;
+ }
+
+ private ZKHelixAdmin(Builder builder) throws IOException, InvalidRoutingDataException {
+ switch (builder.realmMode) {
+ case MULTI_REALM:
+ _zkClient = new FederatedZkClient(builder.realmAwareZkConnectionConfig,
+ builder.realmAwareZkClientConfig);
Review comment:
This should be a try-catch.
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r391406024
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1285,4 +1266,60 @@ public void handleSessionEstablishmentError(Throwable error) throws Exception {
public Long getSessionStartTime() {
return _sessionStartTime;
}
+
+ private RealmAwareZkClient createSingleRealmZkClient() {
Review comment:
- `prepareAndGet` sounds like a method does 2 things. I prefer to name a method which does one thing. `get` is better than `prepareAndGet` as there is no need to care about "prepare" since the final purpose is "get". There is already one method `createClient()` so I prefer to keep the name.
- It is a private method and the logic is pretty straightforward. That's why I didn't consider adding comments. Anyway, since you suggested here, added.
----------------------------------------------------------------
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 #846: [WIP]
Make ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: [WIP] Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r387496746
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -118,6 +118,11 @@ public ConfigAccessor(HelixZkClient zkClient) {
_usesExternalZkClient = true;
}
+ public ConfigAccessor(RealmAwareZkClient zkClient) {
+ _zkClient = zkClient;
+ _usesExternalZkClient = true;
+ }
Review comment:
On a second thought, I forgot to add this constructor. I'll go ahead and work on that 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 #846: [WIP]
Make ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: [WIP] Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r387495110
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -89,33 +91,55 @@
public class ZKHelixAdmin implements HelixAdmin {
+ private static final Logger LOG = LoggerFactory.getLogger(ZKHelixAdmin.class);
+
+
public static final String CONNECTION_TIMEOUT = "helixAdmin.timeOutInSec";
private static final String MAINTENANCE_ZNODE_ID = "maintenance";
private static final int DEFAULT_SUPERCLUSTER_REPLICA = 3;
- private final HelixZkClient _zkClient;
+ private final RealmAwareZkClient _zkClient;
private final ConfigAccessor _configAccessor;
// true if ZKHelixAdmin was instantiated with a HelixZkClient, false otherwise
// This is used for close() to determine how ZKHelixAdmin should close the underlying ZkClient
private final boolean _usesExternalZkClient;
private static Logger logger = LoggerFactory.getLogger(ZKHelixAdmin.class);
- @Deprecated
- public ZKHelixAdmin(HelixZkClient zkClient) {
+ public ZKHelixAdmin() {
+ _zkClient = new FederatedZkClient();
+ _configAccessor = new ConfigAccessor(_zkClient);
+ _usesExternalZkClient = false;
+ }
+
+ public ZKHelixAdmin(RealmAwareZkClient zkClient) {
Review comment:
We could just leave this as HelixZkClient - do you think that'd work?
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r389321897
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -1799,4 +1851,69 @@ private boolean validateWeightForResourceConfig(ClusterConfig clusterConfig,
clusterConfig));
return true;
}
+
+ public static class Builder {
+ private String zkAddress;
+ private RealmAwareZkClient.RealmMode realmMode;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig;
+
+ public Builder() {
+ }
+
+ public ZKHelixAdmin.Builder setZkAddress(String zkAddress) {
+ this.zkAddress = zkAddress;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ this.realmMode = realmMode;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return this;
+ }
+
+ public ZKHelixAdmin build() throws Exception {
+ validate();
+ return new ZKHelixAdmin(this);
+ }
+
+ /*
+ * Validates the given parameters before creating an instance of ZKHelixAdmin.
+ */
+ private void validate() {
+ // Resolve RealmMode based on other parameters
+ boolean isZkAddressSet = zkAddress != null && !zkAddress.isEmpty();
+ if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM && !isZkAddressSet) {
Review comment:
Fixed.
----------------------------------------------------------------
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 #846: [WIP]
Make ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: [WIP] Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r387493543
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -118,6 +118,11 @@ public ConfigAccessor(HelixZkClient zkClient) {
_usesExternalZkClient = true;
}
+ public ConfigAccessor(RealmAwareZkClient zkClient) {
+ _zkClient = zkClient;
+ _usesExternalZkClient = true;
+ }
Review comment:
The new ConfigAccessor should be created with a Builder. I'm going to assume that this is just a placeholder? :)
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r391403485
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1285,4 +1266,60 @@ public void handleSessionEstablishmentError(Throwable error) throws Exception {
public Long getSessionStartTime() {
return _sessionStartTime;
}
+
+ private RealmAwareZkClient createSingleRealmZkClient() {
+ final String shardingKey = buildShardingKey();
+ PathBasedZkSerializer zkSerializer =
+ ChainedPathZkSerializer.builder(new ZNRecordSerializer()).build();
+
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
+ .setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
+ .setZkRealmShardingKey(shardingKey)
+ .setSessionTimeout(_sessionTimeout).build();
+
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig();
+
+ clientConfig.setZkSerializer(zkSerializer)
+ .setConnectInitTimeout(_connectionInitTimeout)
+ .setMonitorType(_instanceType.name())
+ .setMonitorKey(_clusterName)
+ .setMonitorInstanceName(_instanceName)
+ .setMonitorRootPathOnly(isMonitorRootPathOnly());
+
+ if (_instanceType == InstanceType.ADMINISTRATOR) {
+ return buildSelectiveZkClient(SharedZkClientFactory.getInstance(), connectionConfig,
+ clientConfig);
+ }
+
+ return buildSelectiveZkClient(DedicatedZkClientFactory.getInstance(), connectionConfig,
+ clientConfig);
+ }
+
+ private RealmAwareZkClient buildSelectiveZkClient(HelixZkClientFactory zkClientFactory,
Review comment:
Added. Thx.
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r391399404
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -90,33 +92,98 @@
public class ZKHelixAdmin implements HelixAdmin {
+ private static final Logger LOG = LoggerFactory.getLogger(ZKHelixAdmin.class);
+
+
Review comment:
Nit: one empty line would suffice?
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r390792818
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -102,14 +103,13 @@ public AccessResult() {
private static Logger LOG = LoggerFactory.getLogger(ZkBaseDataAccessor.class);
- private final HelixZkClient _zkClient;
+ private final RealmAwareZkClient _zkClient;
// true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
// This is used for close() to determine how ZkBaseDataAccessor should close the underlying
// ZkClient
private final boolean _usesExternalZkClient;
- @Deprecated
- public ZkBaseDataAccessor(HelixZkClient zkClient) {
+ public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
Review comment:
Shouldn't this be 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] pkuwm commented on issue #846: Make ZKHelixAdmin and
ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on issue #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#issuecomment-595873589
> Looks good to me.
>
> Also I think it would be a good idea to test the Builder for HelixAdmin to make sure the Builder creates a working HelixAdmin (or ConfigAccessor, etc.). I am trying to think of ways to do this in other PRs too. Let us think about it.
>
> Totally fine to do that in a separate PR.
As I mentioned, I have the same plan to test and would like to do it for all of the required Java APIs in another PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
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 #846: [WIP]
Make ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: [WIP] Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r387495110
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -89,33 +91,55 @@
public class ZKHelixAdmin implements HelixAdmin {
+ private static final Logger LOG = LoggerFactory.getLogger(ZKHelixAdmin.class);
+
+
public static final String CONNECTION_TIMEOUT = "helixAdmin.timeOutInSec";
private static final String MAINTENANCE_ZNODE_ID = "maintenance";
private static final int DEFAULT_SUPERCLUSTER_REPLICA = 3;
- private final HelixZkClient _zkClient;
+ private final RealmAwareZkClient _zkClient;
private final ConfigAccessor _configAccessor;
// true if ZKHelixAdmin was instantiated with a HelixZkClient, false otherwise
// This is used for close() to determine how ZKHelixAdmin should close the underlying ZkClient
private final boolean _usesExternalZkClient;
private static Logger logger = LoggerFactory.getLogger(ZKHelixAdmin.class);
- @Deprecated
- public ZKHelixAdmin(HelixZkClient zkClient) {
+ public ZKHelixAdmin() {
+ _zkClient = new FederatedZkClient();
+ _configAccessor = new ConfigAccessor(_zkClient);
+ _usesExternalZkClient = false;
+ }
+
+ public ZKHelixAdmin(RealmAwareZkClient zkClient) {
Review comment:
We could just leave this as HelixZkClient - do you think that'd work?
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r391402054
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1285,4 +1266,60 @@ public void handleSessionEstablishmentError(Throwable error) throws Exception {
public Long getSessionStartTime() {
return _sessionStartTime;
}
+
+ private RealmAwareZkClient createSingleRealmZkClient() {
+ final String shardingKey = buildShardingKey();
+ PathBasedZkSerializer zkSerializer =
+ ChainedPathZkSerializer.builder(new ZNRecordSerializer()).build();
+
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
+ .setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
+ .setZkRealmShardingKey(shardingKey)
+ .setSessionTimeout(_sessionTimeout).build();
+
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig();
+
+ clientConfig.setZkSerializer(zkSerializer)
+ .setConnectInitTimeout(_connectionInitTimeout)
+ .setMonitorType(_instanceType.name())
+ .setMonitorKey(_clusterName)
+ .setMonitorInstanceName(_instanceName)
+ .setMonitorRootPathOnly(isMonitorRootPathOnly());
+
+ if (_instanceType == InstanceType.ADMINISTRATOR) {
+ return buildSelectiveZkClient(SharedZkClientFactory.getInstance(), connectionConfig,
+ clientConfig);
+ }
+
+ return buildSelectiveZkClient(DedicatedZkClientFactory.getInstance(), connectionConfig,
+ clientConfig);
+ }
+
+ private RealmAwareZkClient buildSelectiveZkClient(HelixZkClientFactory zkClientFactory,
Review comment:
My suggestion is
- JavaDoc:
Resolves what type of ZkClient this HelixManager should use based on whether MULTI_ZK_ENABLED System config is set or not. Two types of ZkClients are available: 1) If MULTI_ZK_ENABLED is set to true, we create a dedicated RealmAwareZkClient that provides full ZkClient functionalities and connects to the correct ZK by querying MetadataStoreDirectoryService. 2) Otherwise, we create a dedicated HelixZkClient which plainly connects to the ZK address given.
- Rename the method to `resolveZkClient()`
I think this will make our intention more clear.
----------------------------------------------------------------
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] NealSun96 commented on a change in pull request #846: [WIP]
Make ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #846: [WIP] Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r388031459
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1285,4 +1266,85 @@ public void handleSessionEstablishmentError(Throwable error) throws Exception {
public Long getSessionStartTime() {
return _sessionStartTime;
}
+
+ private RealmAwareZkClient buildRealmAwareZkClient() {
+ final String shardingKey = buildShardingKey();
+ PathBasedZkSerializer zkSerializer =
+ ChainedPathZkSerializer.builder(new ZNRecordSerializer()).build();
+
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
+ .setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
+ .setZkRealmShardingKey(shardingKey)
+ .setSessionTimeout(_sessionTimeout).build();
+
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig();
+
+ clientConfig.setZkSerializer(zkSerializer)
+ .setConnectInitTimeout(_connectionInitTimeout)
+ .setMonitorType(_instanceType.name())
+ .setMonitorKey(_clusterName)
+ .setMonitorInstanceName(_instanceName)
+ .setMonitorRootPathOnly(isMonitorRootPathOnly());
+
+ RealmAwareZkClient newClient;
+ if (_instanceType == InstanceType.ADMINISTRATOR) {
+ newClient = buildSharedZkClient(connectionConfig, clientConfig);
+ } else {
+ newClient = buildDedicatedZkClient(connectionConfig, clientConfig);
+ }
+
+ return newClient;
+ }
+
+ private RealmAwareZkClient buildSharedZkClient(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig,
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig) {
+ RealmAwareZkClient zkClient;
+ try {
+ zkClient =
+ SharedZkClientFactory.getInstance().buildZkClient(connectionConfig, clientConfig);
+ } catch (IllegalArgumentException | IOException | InvalidRoutingDataException e) {
+ LOG.warn("Not able to build realm-aware shared ZK client for sharding key: {}, caused by: {}"
+ + " Fallback to HelixZkClient.", connectionConfig.getZkRealmShardingKey(),
+ e.getMessage());
+
+ // Fallback to HelixZkClient
+ HelixZkClient.ZkConnectionConfig helixZkConnectionConfig =
+ new HelixZkClient.ZkConnectionConfig(_zkAddress);
+ helixZkConnectionConfig.setSessionTimeout(connectionConfig.getSessionTimeout());
+ zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(helixZkConnectionConfig, clientConfig.createHelixZkClientConfig());
+ }
+
+ return zkClient;
+ }
+
+ private RealmAwareZkClient buildDedicatedZkClient(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig,
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig) {
+ RealmAwareZkClient zkClient;
+ try {
+ zkClient =
+ DedicatedZkClientFactory.getInstance().buildZkClient(connectionConfig, clientConfig);
+ } catch (IllegalArgumentException | IOException | InvalidRoutingDataException e) {
+ LOG.warn("Not able to build realm-aware Dedicated ZK client for sharding key: {}, caused by: {}"
+ + " Fall back to HelixZkClient.", connectionConfig.getZkRealmShardingKey(),
+ e.getMessage());
+
+ // Fallback to HelixZkClient
+ HelixZkClient.ZkConnectionConfig helixZkConnectionConfig =
+ new HelixZkClient.ZkConnectionConfig(_zkAddress);
+ helixZkConnectionConfig.setSessionTimeout(connectionConfig.getSessionTimeout());
+ zkClient = DedicatedZkClientFactory.getInstance()
+ .buildZkClient(helixZkConnectionConfig, clientConfig.createHelixZkClientConfig());
+ }
+
+ return zkClient;
+ }
+
+ private String buildShardingKey() {
+ return _clusterName.charAt(0) == '/' ? _clusterName : SLASH + _clusterName;
Review comment:
Nit: not much point declaring `SLASH` if it's character counter-part is a local variable.
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r388684854
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -89,33 +91,77 @@
public class ZKHelixAdmin implements HelixAdmin {
+ private static final Logger LOG = LoggerFactory.getLogger(ZKHelixAdmin.class);
+
+
public static final String CONNECTION_TIMEOUT = "helixAdmin.timeOutInSec";
private static final String MAINTENANCE_ZNODE_ID = "maintenance";
private static final int DEFAULT_SUPERCLUSTER_REPLICA = 3;
- private final HelixZkClient _zkClient;
+ private final RealmAwareZkClient _zkClient;
private final ConfigAccessor _configAccessor;
- // true if ZKHelixAdmin was instantiated with a HelixZkClient, false otherwise
+ // true if ZKHelixAdmin was instantiated with a RealmAwareZkClient, false otherwise
// This is used for close() to determine how ZKHelixAdmin should close the underlying ZkClient
private final boolean _usesExternalZkClient;
private static Logger logger = LoggerFactory.getLogger(ZKHelixAdmin.class);
+ /**
+ * @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 ZKHelixAdmin(HelixZkClient zkClient) {
+ public ZKHelixAdmin(RealmAwareZkClient zkClient) {
_zkClient = zkClient;
_configAccessor = new ConfigAccessor(zkClient);
_usesExternalZkClient = true;
}
public ZKHelixAdmin(String zkAddress) {
int timeOutInSec = Integer.parseInt(System.getProperty(CONNECTION_TIMEOUT, "30"));
- HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig();
- clientConfig.setZkSerializer(new ZNRecordSerializer())
- .setConnectInitTimeout(timeOutInSec * 1000);
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress), clientConfig);
- _zkClient.waitUntilConnected(timeOutInSec, TimeUnit.SECONDS);
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig()
+ .setConnectInitTimeout(timeOutInSec * 1000L)
+ .setZkSerializer(new ZNRecordSerializer());
+
+ RealmAwareZkClient zkClient;
+ try {
+ zkClient = new FederatedZkClient(
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(), clientConfig);
+ } catch (IllegalStateException | IOException | InvalidRoutingDataException e) {
+ LOG.info("Not able to connect on multi-realm mode. "
+ + "Connecting on single-realm mode to ZK: {}", zkAddress);
+
+ zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
+ clientConfig.createHelixZkClientConfig());
+ zkClient.waitUntilConnected(timeOutInSec, TimeUnit.SECONDS);
+ }
+
+ _zkClient = zkClient;
+ _configAccessor = new ConfigAccessor(_zkClient);
+ _usesExternalZkClient = false;
+ }
+
+ private ZKHelixAdmin(Builder builder) throws IOException, InvalidRoutingDataException {
+ switch (builder.realmMode) {
+ case MULTI_REALM:
+ _zkClient = new FederatedZkClient(builder.realmAwareZkConnectionConfig,
+ builder.realmAwareZkClientConfig);
Review comment:
Above reply.
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r389321437
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -1799,4 +1851,69 @@ private boolean validateWeightForResourceConfig(ClusterConfig clusterConfig,
clusterConfig));
return true;
}
+
+ public static class Builder {
+ private String zkAddress;
+ private RealmAwareZkClient.RealmMode realmMode;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig;
+
+ public Builder() {
+ }
+
+ public ZKHelixAdmin.Builder setZkAddress(String zkAddress) {
+ this.zkAddress = zkAddress;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ this.realmMode = realmMode;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return this;
+ }
+
+ public ZKHelixAdmin build() throws Exception {
+ validate();
+ return new ZKHelixAdmin(this);
+ }
+
+ /*
+ * Validates the given parameters before creating an instance of ZKHelixAdmin.
+ */
+ 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!");
Review comment:
This is not configAccessor... lets fix the log?
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r391394957
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -1799,4 +1867,73 @@ private boolean validateWeightForResourceConfig(ClusterConfig clusterConfig,
clusterConfig));
return true;
}
+
+ public static class Builder {
Review comment:
Added.
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r391314648
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -1799,4 +1867,73 @@ private boolean validateWeightForResourceConfig(ClusterConfig clusterConfig,
clusterConfig));
return true;
}
+
+ public static class Builder {
Review comment:
Add TODO here. We need to refactor all the builders before merging to the master.
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r391395314
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1285,4 +1266,63 @@ public void handleSessionEstablishmentError(Throwable error) throws Exception {
public Long getSessionStartTime() {
return _sessionStartTime;
}
+
+ private RealmAwareZkClient buildRealmAwareZkClient() {
+ final String shardingKey = buildShardingKey();
+ PathBasedZkSerializer zkSerializer =
+ ChainedPathZkSerializer.builder(new ZNRecordSerializer()).build();
+
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
+ .setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
+ .setZkRealmShardingKey(shardingKey)
+ .setSessionTimeout(_sessionTimeout).build();
+
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig();
+
+ clientConfig.setZkSerializer(zkSerializer)
+ .setConnectInitTimeout(_connectionInitTimeout)
+ .setMonitorType(_instanceType.name())
+ .setMonitorKey(_clusterName)
+ .setMonitorInstanceName(_instanceName)
+ .setMonitorRootPathOnly(isMonitorRootPathOnly());
+
+ RealmAwareZkClient newClient;
+ if (_instanceType == InstanceType.ADMINISTRATOR) {
+ newClient = buildHelixZkClient(SharedZkClientFactory.getInstance(), connectionConfig,
+ clientConfig);
+ } else {
+ newClient = buildHelixZkClient(DedicatedZkClientFactory.getInstance(), connectionConfig,
+ clientConfig);
+ }
+
+ return newClient;
+ }
+
+ private RealmAwareZkClient buildHelixZkClient(HelixZkClientFactory zkClientFactory,
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig,
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig) {
+ if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED)) {
+ try {
+ // Create realm-aware ZkClient.
+ return zkClientFactory.buildZkClient(connectionConfig, clientConfig);
+ } catch (IllegalArgumentException | IOException | InvalidRoutingDataException e) {
+ throw new HelixException("Not able to connect on realm-aware mode for sharding key: "
+ + connectionConfig.getZkRealmShardingKey(), e);
+ }
+ }
+
+ // If multi-zk mode is not enabled, create HelixZkClient with the provided zk address.
+ HelixZkClient.ZkClientConfig helixZkClientConfig = clientConfig.createHelixZkClientConfig();
+ HelixZkClient.ZkConnectionConfig helixZkConnectionConfig =
+ new HelixZkClient.ZkConnectionConfig(_zkAddress)
+ .setSessionTimeout(connectionConfig.getSessionTimeout());
+
+ return zkClientFactory.buildZkClient(helixZkConnectionConfig, helixZkClientConfig);
+ }
Review comment:
I also had the same thought. Based on the original name and the current code structure, I could not find good names. Updated with my new idea. If you have better ideas, please suggest.
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r391402447
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -90,33 +92,98 @@
public class ZKHelixAdmin implements HelixAdmin {
+ private static final Logger LOG = LoggerFactory.getLogger(ZKHelixAdmin.class);
+
+
Review comment:
How could 2 empty lines be added...
Addressed.
----------------------------------------------------------------
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 #846: [WIP]
Make ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: [WIP] Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r387494838
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -89,33 +91,55 @@
public class ZKHelixAdmin implements HelixAdmin {
+ private static final Logger LOG = LoggerFactory.getLogger(ZKHelixAdmin.class);
+
+
public static final String CONNECTION_TIMEOUT = "helixAdmin.timeOutInSec";
private static final String MAINTENANCE_ZNODE_ID = "maintenance";
private static final int DEFAULT_SUPERCLUSTER_REPLICA = 3;
- private final HelixZkClient _zkClient;
+ private final RealmAwareZkClient _zkClient;
private final ConfigAccessor _configAccessor;
// true if ZKHelixAdmin was instantiated with a HelixZkClient, false otherwise
// This is used for close() to determine how ZKHelixAdmin should close the underlying ZkClient
private final boolean _usesExternalZkClient;
private static Logger logger = LoggerFactory.getLogger(ZKHelixAdmin.class);
- @Deprecated
- public ZKHelixAdmin(HelixZkClient zkClient) {
+ public ZKHelixAdmin() {
+ _zkClient = new FederatedZkClient();
+ _configAccessor = new ConfigAccessor(_zkClient);
+ _usesExternalZkClient = false;
Review comment:
We should use a Builder pattern for HelixAdmin here. This is because we need to allow users to provide connection and client configs.
For example, users should be able to set operationRetry time. If we just provide a constructor that doesn't take in any parameters, it won't be very flexible.
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r389321437
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -1799,4 +1851,69 @@ private boolean validateWeightForResourceConfig(ClusterConfig clusterConfig,
clusterConfig));
return true;
}
+
+ public static class Builder {
+ private String zkAddress;
+ private RealmAwareZkClient.RealmMode realmMode;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig;
+
+ public Builder() {
+ }
+
+ public ZKHelixAdmin.Builder setZkAddress(String zkAddress) {
+ this.zkAddress = zkAddress;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ this.realmMode = realmMode;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return this;
+ }
+
+ public ZKHelixAdmin.Builder setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return this;
+ }
+
+ public ZKHelixAdmin build() throws Exception {
+ validate();
+ return new ZKHelixAdmin(this);
+ }
+
+ /*
+ * Validates the given parameters before creating an instance of ZKHelixAdmin.
+ */
+ 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!");
Review comment:
This is not 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 #846: [WIP]
Make ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: [WIP] Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r387494838
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -89,33 +91,55 @@
public class ZKHelixAdmin implements HelixAdmin {
+ private static final Logger LOG = LoggerFactory.getLogger(ZKHelixAdmin.class);
+
+
public static final String CONNECTION_TIMEOUT = "helixAdmin.timeOutInSec";
private static final String MAINTENANCE_ZNODE_ID = "maintenance";
private static final int DEFAULT_SUPERCLUSTER_REPLICA = 3;
- private final HelixZkClient _zkClient;
+ private final RealmAwareZkClient _zkClient;
private final ConfigAccessor _configAccessor;
// true if ZKHelixAdmin was instantiated with a HelixZkClient, false otherwise
// This is used for close() to determine how ZKHelixAdmin should close the underlying ZkClient
private final boolean _usesExternalZkClient;
private static Logger logger = LoggerFactory.getLogger(ZKHelixAdmin.class);
- @Deprecated
- public ZKHelixAdmin(HelixZkClient zkClient) {
+ public ZKHelixAdmin() {
+ _zkClient = new FederatedZkClient();
+ _configAccessor = new ConfigAccessor(_zkClient);
+ _usesExternalZkClient = false;
Review comment:
We should use a Builder pattern for HelixAdmin here. This is because we need to allow users to provide connection and client configs.
For example, users should be able to set operationRetry time. If we just provide a constructor that doesn't take in any parameters, it won't be very flexible.
https://github.com/apache/helix/pull/819 for a 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] narendly merged pull request #846: Make ZKHelixAdmin and
ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly merged pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846
----------------------------------------------------------------
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] NealSun96 commented on a change in pull request #846: [WIP]
Make ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #846: [WIP] Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r388031191
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1285,4 +1266,85 @@ public void handleSessionEstablishmentError(Throwable error) throws Exception {
public Long getSessionStartTime() {
return _sessionStartTime;
}
+
+ private RealmAwareZkClient buildRealmAwareZkClient() {
+ final String shardingKey = buildShardingKey();
+ PathBasedZkSerializer zkSerializer =
+ ChainedPathZkSerializer.builder(new ZNRecordSerializer()).build();
+
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
+ .setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
+ .setZkRealmShardingKey(shardingKey)
+ .setSessionTimeout(_sessionTimeout).build();
+
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig();
+
+ clientConfig.setZkSerializer(zkSerializer)
+ .setConnectInitTimeout(_connectionInitTimeout)
+ .setMonitorType(_instanceType.name())
+ .setMonitorKey(_clusterName)
+ .setMonitorInstanceName(_instanceName)
+ .setMonitorRootPathOnly(isMonitorRootPathOnly());
+
+ RealmAwareZkClient newClient;
+ if (_instanceType == InstanceType.ADMINISTRATOR) {
+ newClient = buildSharedZkClient(connectionConfig, clientConfig);
+ } else {
+ newClient = buildDedicatedZkClient(connectionConfig, clientConfig);
+ }
+
+ return newClient;
+ }
+
+ private RealmAwareZkClient buildSharedZkClient(
Review comment:
Is it possible to combine `buildSharedZkClient` and `buildDedicatedZkClient` to reduce duplication? Both instances of the factories extend `HelixZkClientFactory`.
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r391400730
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -117,7 +121,7 @@
private final String _version;
private int _reportLatency;
- protected HelixZkClient _zkclient = null;
+ protected RealmAwareZkClient _zkclient = null;
Review comment:
Nit: I know we don't have to change this, but initializing to null is redundant.
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r389205431
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -89,33 +91,88 @@
public class ZKHelixAdmin implements HelixAdmin {
+ private static final Logger LOG = LoggerFactory.getLogger(ZKHelixAdmin.class);
+
+
public static final String CONNECTION_TIMEOUT = "helixAdmin.timeOutInSec";
private static final String MAINTENANCE_ZNODE_ID = "maintenance";
private static final int DEFAULT_SUPERCLUSTER_REPLICA = 3;
- private final HelixZkClient _zkClient;
+ private final RealmAwareZkClient _zkClient;
private final ConfigAccessor _configAccessor;
- // true if ZKHelixAdmin was instantiated with a HelixZkClient, false otherwise
+ // true if ZKHelixAdmin was instantiated with a RealmAwareZkClient, false otherwise
// This is used for close() to determine how ZKHelixAdmin should close the underlying ZkClient
private final boolean _usesExternalZkClient;
private static Logger logger = LoggerFactory.getLogger(ZKHelixAdmin.class);
+ /**
+ * @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 ZKHelixAdmin(HelixZkClient zkClient) {
+ public ZKHelixAdmin(RealmAwareZkClient zkClient) {
_zkClient = zkClient;
_configAccessor = new ConfigAccessor(zkClient);
_usesExternalZkClient = true;
}
public ZKHelixAdmin(String zkAddress) {
int timeOutInSec = Integer.parseInt(System.getProperty(CONNECTION_TIMEOUT, "30"));
- HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig();
- clientConfig.setZkSerializer(new ZNRecordSerializer())
- .setConnectInitTimeout(timeOutInSec * 1000);
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress), clientConfig);
- _zkClient.waitUntilConnected(timeOutInSec, TimeUnit.SECONDS);
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig()
+ .setConnectInitTimeout(timeOutInSec * 1000L)
+ .setZkSerializer(new ZNRecordSerializer());
+
+ RealmAwareZkClient zkClient;
+ try {
+ zkClient = new FederatedZkClient(
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(), clientConfig);
+ } catch (IllegalStateException | IOException | InvalidRoutingDataException e) {
+ LOG.info("Not able to connect on multi-realm mode. "
+ + "Connecting on single-realm mode to ZK: {}", zkAddress);
+
+ zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
+ clientConfig.createHelixZkClientConfig());
+ zkClient.waitUntilConnected(timeOutInSec, TimeUnit.SECONDS);
+ }
+
+ _zkClient = zkClient;
+ _configAccessor = new ConfigAccessor(_zkClient);
+ _usesExternalZkClient = false;
+ }
+
+ private ZKHelixAdmin(Builder builder) {
+ RealmAwareZkClient zkClient;
+ switch (builder.realmMode) {
+ case MULTI_REALM:
+ try {
+ zkClient = new FederatedZkClient(builder.realmAwareZkConnectionConfig,
+ builder.realmAwareZkClientConfig);
+ 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);
Review comment:
@pkuwm
Please throw a HelixException here and propagate the exception.
Also note that per our offline discussion, there will no longer be a fallback to single realm mode. If multi-realm fails, we fail the creation.
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r388754699
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1285,4 +1266,62 @@ public void handleSessionEstablishmentError(Throwable error) throws Exception {
public Long getSessionStartTime() {
return _sessionStartTime;
}
+
+ private RealmAwareZkClient buildRealmAwareZkClient() {
+ final String shardingKey = buildShardingKey();
+ PathBasedZkSerializer zkSerializer =
+ ChainedPathZkSerializer.builder(new ZNRecordSerializer()).build();
+
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
+ .setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
+ .setZkRealmShardingKey(shardingKey)
+ .setSessionTimeout(_sessionTimeout).build();
+
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig();
+
+ clientConfig.setZkSerializer(zkSerializer)
+ .setConnectInitTimeout(_connectionInitTimeout)
+ .setMonitorType(_instanceType.name())
+ .setMonitorKey(_clusterName)
+ .setMonitorInstanceName(_instanceName)
+ .setMonitorRootPathOnly(isMonitorRootPathOnly());
+
+ RealmAwareZkClient newClient;
+ if (_instanceType == InstanceType.ADMINISTRATOR) {
+ newClient = buildHelixZkClient(SharedZkClientFactory.getInstance(), connectionConfig,
+ clientConfig);
+ } else {
+ newClient = buildHelixZkClient(DedicatedZkClientFactory.getInstance(), connectionConfig,
+ clientConfig);
+ }
+
+ return newClient;
+ }
+
+ private RealmAwareZkClient buildHelixZkClient(HelixZkClientFactory zkClientFactory,
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig,
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig) {
+ try {
+ return zkClientFactory.buildZkClient(connectionConfig, clientConfig);
+ } catch (IllegalArgumentException | IOException | InvalidRoutingDataException e) {
+ LOG.info("Not able to connect on realm-aware mode for sharding key: {}, caused by: {} ."
+ + "Trying to connect to ZK: {}.", connectionConfig.getZkRealmShardingKey(),
+ e.getMessage(), _zkAddress);
+ }
+
+ // Fall back to HelixZkClient
+ HelixZkClient.ZkClientConfig helixZkClientConfig = clientConfig.createHelixZkClientConfig();
+ HelixZkClient.ZkConnectionConfig helixZkConnectionConfig =
+ new HelixZkClient.ZkConnectionConfig(_zkAddress)
+ .setSessionTimeout(connectionConfig.getSessionTimeout());
+
+ return zkClientFactory.buildZkClient(helixZkConnectionConfig, helixZkClientConfig);
+ }
+
Review comment:
Nit: remove extra line
----------------------------------------------------------------
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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r391034890
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -102,14 +103,13 @@ public AccessResult() {
private static Logger LOG = LoggerFactory.getLogger(ZkBaseDataAccessor.class);
- private final HelixZkClient _zkClient;
+ private final RealmAwareZkClient _zkClient;
// true if ZkBaseDataAccessor was instantiated with a HelixZkClient, false otherwise
// This is used for close() to determine how ZkBaseDataAccessor should close the underlying
// ZkClient
private final boolean _usesExternalZkClient;
- @Deprecated
- public ZkBaseDataAccessor(HelixZkClient zkClient) {
+ public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
Review comment:
Good catch. It should be. When I started working on it, I was not sure and thought we would keep the new `RealmAwareZkClient ` and deprecate `HelixZkClient ` instead. I already deprecated in the ZkBaseDataAccessor PR.
Updated to deprecate 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 #846: Make
ZKHelixAdmin and ZKHelixManager Realm-aware
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #846: Make ZKHelixAdmin and ZKHelixManager Realm-aware
URL: https://github.com/apache/helix/pull/846#discussion_r388513655
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -89,33 +91,77 @@
public class ZKHelixAdmin implements HelixAdmin {
+ private static final Logger LOG = LoggerFactory.getLogger(ZKHelixAdmin.class);
+
+
public static final String CONNECTION_TIMEOUT = "helixAdmin.timeOutInSec";
private static final String MAINTENANCE_ZNODE_ID = "maintenance";
private static final int DEFAULT_SUPERCLUSTER_REPLICA = 3;
- private final HelixZkClient _zkClient;
+ private final RealmAwareZkClient _zkClient;
private final ConfigAccessor _configAccessor;
- // true if ZKHelixAdmin was instantiated with a HelixZkClient, false otherwise
+ // true if ZKHelixAdmin was instantiated with a RealmAwareZkClient, false otherwise
// This is used for close() to determine how ZKHelixAdmin should close the underlying ZkClient
private final boolean _usesExternalZkClient;
private static Logger logger = LoggerFactory.getLogger(ZKHelixAdmin.class);
+ /**
+ * @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 ZKHelixAdmin(HelixZkClient zkClient) {
+ public ZKHelixAdmin(RealmAwareZkClient zkClient) {
_zkClient = zkClient;
_configAccessor = new ConfigAccessor(zkClient);
_usesExternalZkClient = true;
}
public ZKHelixAdmin(String zkAddress) {
int timeOutInSec = Integer.parseInt(System.getProperty(CONNECTION_TIMEOUT, "30"));
- HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig();
- clientConfig.setZkSerializer(new ZNRecordSerializer())
- .setConnectInitTimeout(timeOutInSec * 1000);
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress), clientConfig);
- _zkClient.waitUntilConnected(timeOutInSec, TimeUnit.SECONDS);
+ RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
+ new RealmAwareZkClient.RealmAwareZkClientConfig()
+ .setConnectInitTimeout(timeOutInSec * 1000L)
+ .setZkSerializer(new ZNRecordSerializer());
+
+ RealmAwareZkClient zkClient;
+ try {
+ zkClient = new FederatedZkClient(
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build(), clientConfig);
+ } catch (IllegalStateException | IOException | InvalidRoutingDataException e) {
+ LOG.info("Not able to connect on multi-realm mode. "
+ + "Connecting on single-realm mode to ZK: {}", zkAddress);
+
+ zkClient = SharedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
+ clientConfig.createHelixZkClientConfig());
+ zkClient.waitUntilConnected(timeOutInSec, TimeUnit.SECONDS);
+ }
+
+ _zkClient = zkClient;
+ _configAccessor = new ConfigAccessor(_zkClient);
+ _usesExternalZkClient = false;
+ }
+
+ private ZKHelixAdmin(Builder builder) throws IOException, InvalidRoutingDataException {
Review comment:
Why is the constructor throwing all sorts of exceptions? As we discussed, this is where we catch checked exceptions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org