narendly commented on a change in pull request #1127:
URL: https://github.com/apache/helix/pull/1127#discussion_r447393791
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/GenericBaseDataAccessorBuilder.java
##########
@@ -123,23 +124,26 @@ protected RealmAwareZkClient
createZkClient(RealmAwareZkClient.RealmMode realmMo
/**
* Validate ZkClientType based on RealmMode.
+ * If ZkClientType is DEDICATED or SHARED, the realm mode must be
SINGLE-REALM.
+ * If ZkClientType is FEDERATED, the realm mode must be MULTI-REALM.
* @param zkClientType
* @param realmMode
*/
private void validateZkClientType(ZkClientType zkClientType,
RealmAwareZkClient.RealmMode realmMode) {
- boolean isZkClientTypeSet = zkClientType != null;
- // If ZkClientType is set, RealmMode must either be single-realm or not
set.
- if (isZkClientTypeSet && realmMode ==
RealmAwareZkClient.RealmMode.MULTI_REALM) {
- throw new HelixException("ZkClientType cannot be set on multi-realm
mode!");
+ if (realmMode == null) {
Review comment:
In `GenericZkHelixApiBuilder.java::validate()`, we do this:
` if (_realmMode == null) {
_realmMode = isZkAddressSet ? RealmAwareZkClient.RealmMode.SINGLE_REALM
: RealmAwareZkClient.RealmMode.MULTI_REALM;
}`
And `GenericZkHelixApiBuilder.java::validate()` is called before this
function. So we can assume that `realmMode` will never be null. I'm glad you
asked this question - I will clarify this in the code and the JavaDoc.
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/ZkClientType.java
##########
@@ -25,18 +25,24 @@
* creation, callback functionality, and session management. But note that
this is more
* resource-heavy since it creates a dedicated ZK connection so should be
used sparingly only
* when the aforementioned features are needed.
+ *
+ * Valid on SINGLE_REALM only.
*/
DEDICATED,
Review comment:
@jiajunwang
The user does have the ability to select the ZkClientType. Before
ZooScalability, we allowed users to select the ZkClientType (Dedicated or
Shared) when creating a BaseDataAccessor. We are doing this to keep things
backward-compatible. See the following examples (copy-pasted from the test
cases added as part of this PR) below:
```
// Create ZkBaseDataAccessor, multi-realm, dedicated ZkClient (should
fail)
try {
accessor =
zkBaseDataAccessorBuilder.setRealmMode(RealmAwareZkClient.RealmMode.MULTI_REALM)
.setZkClientType(ZkClientType.DEDICATED).setZkAddress(firstZkAddress)
.setRealmAwareZkConnectionConfig(connectionConfigBuilder.build()).build();
Assert.fail("MULTI_REALM and DEDICATED ZkClientType are an invalid
combination!");
} catch (HelixException e) {
// Expected
}
// Create ZkBaseDataAccessor, multi-realm, shared ZkClient (should fail)
try {
accessor =
zkBaseDataAccessorBuilder.setRealmMode(RealmAwareZkClient.RealmMode.MULTI_REALM)
.setZkClientType(ZkClientType.SHARED).setZkAddress(firstZkAddress)
.setRealmAwareZkConnectionConfig(connectionConfigBuilder.build()).build();
Assert.fail("MULTI_REALM and SHARED ZkClientType are an invalid
combination!");
} catch (HelixException e) {
// Expected
}
// Create ZkBaseDataAccessor, multi-realm, federated ZkClient,
ZkAddress set (should fail)
try {
accessor =
zkBaseDataAccessorBuilder.setRealmMode(RealmAwareZkClient.RealmMode.MULTI_REALM)
.setZkClientType(ZkClientType.FEDERATED).setZkAddress(firstZkAddress)
.setRealmAwareZkConnectionConfig(connectionConfigBuilder.build()).build();
Assert.fail("MULTI_REALM and FEDERATED ZkClientType do not connect to
one ZK!");
} catch (HelixException e) {
// Expected
}
```
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/GenericBaseDataAccessorBuilder.java
##########
@@ -123,23 +124,26 @@ protected RealmAwareZkClient
createZkClient(RealmAwareZkClient.RealmMode realmMo
/**
* Validate ZkClientType based on RealmMode.
+ * If ZkClientType is DEDICATED or SHARED, the realm mode must be
SINGLE-REALM.
+ * If ZkClientType is FEDERATED, the realm mode must be MULTI-REALM.
* @param zkClientType
* @param realmMode
*/
private void validateZkClientType(ZkClientType zkClientType,
RealmAwareZkClient.RealmMode realmMode) {
- boolean isZkClientTypeSet = zkClientType != null;
- // If ZkClientType is set, RealmMode must either be single-realm or not
set.
- if (isZkClientTypeSet && realmMode ==
RealmAwareZkClient.RealmMode.MULTI_REALM) {
- throw new HelixException("ZkClientType cannot be set on multi-realm
mode!");
+ if (realmMode == null) {
Review comment:
As seen in:
```
protected void validate() {
super.validate();
validateZkClientType(_zkClientType, _realmMode);
}
```
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
