[GitHub] [helix] NealSun96 commented on a change in pull request #1460: Allow Stale Reply Messages to Pass without Exceptions

[GitBox <gi...@apache.org>]

NealSun96 commented on a change in pull request #1460:
URL: https://github.com/apache/helix/pull/1460#discussion_r504128305



##########
File path: helix-core/src/main/java/org/apache/helix/messaging/handling/AsyncCallbackService.java
##########
@@ -113,12 +104,19 @@ public HelixTaskResult handleMessage() throws InterruptedException {
           + _correlationId);
 
       AsyncCallback callback = _callbackMap.get(_correlationId);
-      synchronized (callback) {
-        callback.onReply(_message);
-        if (callback.isDone()) {
-          _logger.info("Removing finished callback, correlationid:" + _correlationId);
-          _callbackMap.remove(_correlationId);
+      if (callback != null) {
+        synchronized (callback) {
+          callback.onReply(_message);
+          if (callback.isDone()) {
+            _logger.info("Removing finished callback, correlationid:" + _correlationId);
+            _callbackMap.remove(_correlationId);
+          }
         }
+      } else {
+        String msg = "Message " + _message.getMsgId()
+            + " does not have correponding callback. Probably timed out already. Correlation id: "
+            + _correlationId;
+        _logger.warn(msg);

Review comment:
       You mean they wouldn't be able to distinguish "stale messages" vs "messages that don't have callbacks set up correctly"? I think these cases are logically the same and can't be easily distinguished code-level. If we are loosening up on one case, it's going to affect the other. 
   
   On the other hand, callback registration is done in our code and naturally there should be a callback before sending messages that require replies. I imagine the previous check was there for cautionary purpose, not that messages are commonly created with incorrect correlation ids. 




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org