kaisun2000 commented on a change in pull request #1540:
URL: https://github.com/apache/helix/pull/1540#discussion_r526500354
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -340,26 +340,28 @@ public void enqueueTask(NotificationContext
changeContext) throws Exception {
public void invoke(NotificationContext changeContext) throws Exception {
Type type = changeContext.getType();
long start = System.currentTimeMillis();
+ if (logger.isInfoEnabled()) {
+ logger.info("{} START: CallbackHandler {}, INVOKE {} listener: {} type:
{}",
+ Thread.currentThread().getId(), _uid, _path, _listener, type);
+ }
- // This allows the listener to work with one change at a time
- synchronized (_manager) {
- if (logger.isInfoEnabled()) {
- logger
- .info("{} START: CallbackHandler {}, INVOKE {} listener: {} type:
{}", Thread.currentThread().getId(),
- _uid, _path, _listener, type);
- }
-
+ // TODO: Having subscribeForChanges here might be overkill. Maybe it could
be moved out later.
Review comment:
Why do we sync on this here? In fact, how do we reason it?
Assuming no batch, the calling path would be from enqueueTask(), which is
called by handleXXX from ZkClient event thread. And also init() and reset()
Assuming batch, the calling path would be from _batchCallbackProcessor and
also init() and reset().
init() is further called by either first time CallbackHandler construction
or ZK session change.
It seems that ZK session change is the only places that there would be race
condition, right?
----------------------------------------------------------------
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]
