narendly commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431483789
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1256,14 +1256,18 @@ protected void
checkLiveInstancesObservation(List<LiveInstance> liveInstances,
for (String instance : curInstances.keySet()) {
if (lastInstances == null || !lastInstances.containsKey(instance)) {
- try {
- manager.addCustomizedStateRootChangeListener(this, instance);
- logger.info(manager.getInstanceName() + " added customized root
change listener for"
- + " " + instance
- + ", listener: " + this);
- } catch (Exception e) {
- logger.error("Fail to add customized root change listener for
instance: " + instance,
- e);
+ PropertyKey propertyKey =
+ keyBuilder.customizedStatesRoot(instance);
+ if (propertyKey.getPath() != null) {
Review comment:
You're creating a PropertyKey object with the given instance name and
checking whether it's null? How would it be null if you've just created the
path?
Did you mean to check whether the path exists in ZooKeeper? If so, shouldn't
you do an exist() check with a ZkClient-based API?
Unless I'm mistaken and we actually somehow pre-populate the KeyBuilder with
a whitelist of instances for the CustomizedView feature?
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1256,14 +1256,18 @@ protected void
checkLiveInstancesObservation(List<LiveInstance> liveInstances,
for (String instance : curInstances.keySet()) {
if (lastInstances == null || !lastInstances.containsKey(instance)) {
- try {
- manager.addCustomizedStateRootChangeListener(this, instance);
- logger.info(manager.getInstanceName() + " added customized root
change listener for"
- + " " + instance
- + ", listener: " + this);
- } catch (Exception e) {
- logger.error("Fail to add customized root change listener for
instance: " + instance,
- e);
+ PropertyKey propertyKey =
+ keyBuilder.customizedStatesRoot(instance);
+ if (propertyKey.getPath() != null) {
+ try {
+ manager.addCustomizedStateRootChangeListener(this, instance);
+ logger.info(
+ manager.getInstanceName() + " added customized root change
listener for" + " " + instance
+ + ", listener: " + this);
+ } catch (Exception e) {
+ logger.error("Fail to add customized root change listener for
instance: " + instance,
Review comment:
"customized root change" is confusing. Could we explicitly state what it
is? For example,
"Added (or Failed to add) the root path for Customized State ..."
Also, this log doesn't seem entirely necessary. Don't need to log every time
we add a listener - I'd make this a debug log so as not to pollute 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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
