kaisun2000 commented on a change in pull request #1035:
URL: https://github.com/apache/helix/pull/1035#discussion_r431602293
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -752,6 +756,10 @@ public void handleChildChange(String parentPath,
List<String> currentChilds) {
changeContext.setType(NotificationContext.Type.CALLBACK);
changeContext.setPathChanged(parentPath);
changeContext.setChangeType(_changeType);
+ if (!isReady()) {
+ // avoid leaking CallbackHandler
+ return;
+ }
Review comment:
This I thought about it carefully before. This is exactly what we want,
no handling the event in this case.
First, we can reason it this way. "no ready" is either the callbackhandler
is never initialized, or finalized. (Confirmed by examining the code setting
isReady to false). In both cases, the callbackhandler should meet the invariant
that they don't subscribe to changes from Zookeeper, they don't put them onto
ZkClient's data/child listener lists. Thus, they don't handle event and invoke
further callback in the callbackhandler. In fact, they can be trigger-ed only
in the fireAllEvent's lambda. (Long term, we should rethink about fireAllEvent.)
Second, that is what previous developer intended to do also. See the
following code, the previous developer also skip all the rest processing.
```
public void enqueueTask(NotificationContext changeContext) throws Exception {
// async mode only applicable to CALLBACK from ZK, During INIT and
FINALIZE invoke the
// callback's immediately.
if (_batchModeEnabled && changeContext.getType() ==
NotificationContext.Type.CALLBACK) {
logger.debug("Enqueuing callback");
if (!isReady()) {
logger.info("CallbackHandler is not ready, ignore change callback
from path: " + _path
+ ", for listener: " + _listener);
} else {
synchronized (this) {
if (_batchCallbackProcessor != null) {
_batchCallbackProcessor.queueEvent(changeContext.getType(),
changeContext);
} else {
throw new HelixException(
"Failed to process callback in batch mode. Batch Callback
Processor does not exist.");
}
}
}
} else {
invoke(changeContext);
}
```
Of course, previous batchProcessor or CallbackHandler-AsycSubscribe have a
deadly drawback. They violate the important invariant -- **the code need to
install the watch before or at the same time when reading the current snapshot
of data upon watch notification in order not to lose update.**
This is the reason they lose update. And this is probably also the reason we
added line 763 to do subscribeForChanges() synchronously.
----------------------------------------------------------------
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]
