kaisun2000 commented on a change in pull request #1489:
URL: https://github.com/apache/helix/pull/1489#discussion_r513842032
##########
File path:
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java
##########
@@ -997,30 +873,186 @@ public void onMessage(String instanceName, List<Message>
messages,
}
}
- // update message state to READ in batch and schedule all read messages
+ // update message state to READ in batch and schedule tasks for all read
messages
if (readMsgs.size() > 0) {
updateMessageState(readMsgs, accessor, instanceName);
- // Remove message if schedule tasks are failed.
for (Map.Entry<String, MessageHandler> handlerEntry :
stateTransitionHandlers.entrySet()) {
MessageHandler handler = handlerEntry.getValue();
NotificationContext context =
stateTransitionContexts.get(handlerEntry.getKey());
- Message msg = handler._message;
- if (!scheduleTask(new HelixTask(msg, context, handler, this))) {
- removeMessageFromTaskAndFutureMap(msg);
- removeMessageFromZK(accessor, msg, instanceName);
- }
+ scheduleTaskForMessage(instanceName, accessor, handler, context);
}
for (int i = 0; i < nonStateTransitionHandlers.size(); i++) {
MessageHandler handler = nonStateTransitionHandlers.get(i);
NotificationContext context = nonStateTransitionContexts.get(i);
- Message msg = handler._message;
- if (!scheduleTask(new HelixTask(msg, context, handler, this))) {
- removeMessageFromTaskAndFutureMap(msg);
- removeMessageFromZK(accessor, msg, instanceName);
+ scheduleTaskForMessage(instanceName, accessor, handler, context);
+ }
+ }
+ }
+
+ /**
+ * Inspect the message. Report and remove it if no operation needs to be
done.
+ * @param message
+ * @param instanceName
+ * @param changeContext
+ * @param manager
+ * @param sessionId
+ * @param stateTransitionHandlers
+ * @return True if the message is no-op message and no other process step is
required.
+ */
+ private boolean checkForNoOpMessage(Message message, String instanceName,
Review comment:
Right now, the code is actually within a big try{} block. the catch
block would remove message. I know in the next diff you will do more magic to
make sure when this method exit, they must be a statemodel with some state.
Currently, there is no logic error (probably the only thing is the first line
of getHelixDataAcessor() is one is super picky). However, state this variant
would help later developers to understand the reasoning here. So I think it
would be a good idea to have a comment 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]
