narendly commented on a change in pull request #1127:
URL: https://github.com/apache/helix/pull/1127#discussion_r447951068
##########
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:
1. I think you're referring to
```
case MULTI_REALM:
try {
if (_zkClientType == ZkClientType.DEDICATED) {
// Use a realm-aware dedicated zk client
zkClient = DedicatedZkClientFactory.getInstance()
.buildZkClient(connectionConfig, clientConfig);
} else if (_zkClientType == ZkClientType.SHARED) {
// Use a realm-aware shared zk client
zkClient =
SharedZkClientFactory.getInstance().buildZkClient(connectionConfig,
clientConfig);
} else {
zkClient = new FederatedZkClient(connectionConfig, clientConfig);
}
} catch (IOException | InvalidRoutingDataException |
IllegalStateException e) {
throw new HelixException("Not able to connect on multi-realm
mode.", e);
}
break;
```
Reviewing this logic again, I believe this was a bug. Multi-realm means it
has to use FederatedZkClient only because it has to be able to talk to multiple
ZKs. I've fixed this. Good catch!
2. I've removed some code in the createZkClient() method.
3. We can still support ZkClientType after fixing the bug. Like you said,
ZkClientType is not set, we default to SHARED (original behavior).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
