xyuanlu commented on a change in pull request #1540:
URL: https://github.com/apache/helix/pull/1540#discussion_r526520539
##########
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:
Please correct me if I am wrong. With batch enabled, it is possible that
we get 'Finalize' and a 'callback' at the same time. If there is no lock in
line 348 to 358, then it is possible that we do
1. type check first for 'callback' and passed,
2. type check for 'Finalize' and passed,
3. unsubscribe for 'Finalize'
4. Subscribe for 'callback'
There won't be such issue with the lock here.
----------------------------------------------------------------
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]
