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 skips all the rest processing.
```java
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
singleton have a deadly drawback. They violate the important invariant -- **the
code needs 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 batchProcessor or CallbackHandler-AsycSubscribe singleton
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]
