This is an automated email from the ASF dual-hosted git repository. hzlu pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/helix.git The following commit(s) were added to refs/heads/master by this push: new b0b9911 Reduce unnecessary getChildren call in subscribeForChanges (#1344) b0b9911 is described below commit b0b9911d803227c2595d19a0c1d894f996848ce6 Author: Huizhi Lu AuthorDate: Thu Sep 24 23:40:41 2020 -0700 Reduce unnecessary getChildren call in subscribeForChanges (#1344) In CallbackHanlder's subscribeForChanges(), children names need to be fetched. subscribeChildChange() does actually have the children list, but it doesn't return, and later getChildren() is called again. It wastes one zk call: not only wasting time in handling a callback, but also adds more read pressure to zk server. This commit uses the return result from subscribeChildChange() and saves one getChildren() call. --- .../apache/helix/manager/zk/CallbackHandler.java | 39 +++++++++++++--------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java b/helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java index 2e8726d..dece3de 100644 --- a/helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java +++ b/helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java @@ -538,7 +538,12 @@ public class CallbackHandler implements IZkChildListener, IZkDataListener { } } - private void subscribeChildChange(String path, NotificationContext.Type callbackType) { + /* + * If callback type is INIT or CALLBACK, subscribes child change listener to the path + * and returns the path's children names. The children list might be null when the path + * doesn't exist or callback type is INIT/CALLBACK. + */ + private List subscribeChildChange(String path, NotificationContext.Type callbackType) { if (callbackType == NotificationContext.Type.INIT || callbackType == NotificationContext.Type.CALLBACK) { if (logger.isDebugEnabled()) { @@ -551,20 +556,26 @@ public class CallbackHandler implements IZkChildListener, IZkDataListener { // Later, CALLBACK type, the CallbackHandler already registered the watch and knows the // path was created. Here, to avoid leaking path in ZooKeeper server, we would not let // CallbackHandler to install exists watch, namely watch for path not existing. - // Note when path is removed, the CallbackHanler would remove itself from ZkHelixManager too + // Note when path is removed, the CallbackHandler would remove itself from ZkHelixManager too // to avoid leaking a CallbackHandler. - ChildrenSubscribeResult childrenSubscribeResult = _zkClient.subscribeChildChanges(path, this, callbackType != Type.INIT); + ChildrenSubscribeResult childrenSubscribeResult = + _zkClient.subscribeChildChanges(path, this, callbackType != Type.INIT); logger.debug("CallbackHandler {} subscribe data path {} result {}", _uid, path, childrenSubscribeResult.isInstalled()); if (!childrenSubscribeResult.isInstalled()) { logger.info("CallbackHandler {} subscribe data path {} failed!", _uid, path); } + // getChildren() might be null: when path doesn't exist. + return childrenSubscribeResult.getChildren(); } else if (callbackType == NotificationContext.Type.FINALIZE) { logger.info("CallbackHandler{}, {} unsubscribe child-change. path: {}, listener: {}", _uid ,_manager.getInstanceName(), path, _listener); _zkClient.unsubscribeChildChanges(path, this); } + + // List of children could be empty, but won't be null. + return _zkClient.getChildren(path); } private void subscribeDataChange(String path, NotificationContext.Type callbackType) { @@ -597,23 +608,21 @@ public class CallbackHandler implements IZkChildListener, IZkDataListener { private void subscribeForChanges(NotificationContext.Type callbackType, String path, boolean watchChild) { - logger.info("CallbackHandler {} Subscribing changes listener to path: {}, type: {}, listener: {}", - _uid, path, callbackType, _listener); + logger.info("CallbackHandler {} subscribing changes listener to path: {}, callback type: {}, " + + "event types: {}, listener: {}, watchChild: {}", + _uid, path, callbackType, _eventTypes, _listener, watchChild); long start = System.currentTimeMillis(); if (_eventTypes.contains(EventType.NodeDataChanged) || _eventTypes.contains(EventType.NodeCreated) || _eventTypes.contains(EventType.NodeDeleted)) { - logger.info("CallbackHandler{} Subscribing data change listener to path: {}", _uid, path); + logger.info("CallbackHandler {} subscribing data change listener to path: {}", _uid, path); subscribeDataChange(path, callbackType); } if (_eventTypes.contains(EventType.NodeChildrenChanged)) { - logger.info("CallbackHandler{}, Subscribing child change listener to path: {}", _uid, path); - subscribeChildChange(path, callbackType); + List children = subscribeChildChange(path, callbackType); if (watchChild) { - logger.info("CallbackHandler{}, Subscribing data change listener to all children for path: {}", _uid, path); - try { switch (_changeType) { case CURRENT_STATE: @@ -633,11 +642,10 @@ public class CallbackHandler implements IZkChildListener, IZkDataListener { if (bucketSize > 0) { // subscribe both data-change and child-change on bucketized parent node // data-change gives a delete-callback which is used to remove watch - subscribeChildChange(childPath, callbackType); + List bucketizedChildNames = subscribeChildChange(childPath, callbackType); subscribeDataChange(childPath, callbackType); // subscribe data-change on bucketized child - List bucketizedChildNames = _zkClient.getChildren(childPath); if (bucketizedChildNames != null) { for (String bucketizedChildName : bucketizedChildNames) { String bucketizedChildPath = childPath + "/" + bucketizedChildName; @@ -651,10 +659,9 @@ public class CallbackHandler implements IZkChildListener, IZkDataListener { break; } default: { - List childNames = _zkClient.getChildren(path); - if (childNames != null) { - for (String childName : childNames) { - String childPath = path + "/" + childName; + if (children != null) { + for (String child : children) { + String childPath = path + "/" + child; subscribeDataChange(childPath, callbackType); } }