Murtadha Hubail has posted comments on this change. Change subject: Introduce MessagingNetworkManager for NC2NC AppMessaging ......................................................................
Patch Set 4: (10 comments) https://asterix-gerrit.ics.uci.edu/#/c/897/4/asterixdb/asterix-app/src/main/java/org/apache/asterix/messaging/NCMessageBroker.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/messaging/NCMessageBroker.java: Line 292: public void run() { > It seems that this never stops. Would there be an issue, if we leave the lo It depends on how NC2NC messaging will be used. If there is some kind of messaging that will be done to coordinate nodes shutdown during the JVM shutdown hook, then it is possible that this thread might get interrupted (and stopped) before these messages arrive from other nodes, causing the shutdown progress to stop forever. For example, in your WIP change (https://asterix-gerrit.ics.uci.edu/#/c/847/7), you try to properly stop the TransactionSubsystem by interrupting the checkpoint thread sleep and on catching the InterruptedException you return from the loop, and finally wait for the checkpoint thread to return on TransactionSubsystem using join(). Everything would work perfectly if the checkpoint thread was actually sleeping when it is interrupted. However, if the checkpoint thread was actually processing something and not sleeping, the join() call in the TransactionSubsystem will cause the NC shutdown to hang forever. The proper way to stop this thread gracefully would be to pass a POSION_PILL to its queue when it is no longer needed. An even better way is use Callables to propagate exceptions that could be thrown in the graceful shutdown of the thread. Since I don't know the usage of NC2NC yet, I left it like this to be on the safe side, and this thread will be killed by the end of the shutdown hook when the process terminates. https://asterix-gerrit.ics.uci.edu/#/c/897/4/asterixdb/asterix-app/src/main/java/org/apache/asterix/messaging/NCMessagingHelper.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/messaging/NCMessagingHelper.java: Line 33: import org.apache.asterix.external.feed.management.ConcurrentFramePool; > I think that we shouldn't depend on the external package here. Can we move I moved it to asterix-common along with its test class. Line 50: //TODO make these values configurable and account for their memory usage > Yes, please. Done https://asterix-gerrit.ics.uci.edu/#/c/897/4/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/messaging/api/INCMessageBroker.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/messaging/api/INCMessageBroker.java: Line 49: public void reportMaxResourceId() throws Exception; > I know that it's not part of this change, but do we still need this method Removed. https://asterix-gerrit.ics.uci.edu/#/c/897/4/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/comm/IChannelReadInterface.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/comm/IChannelReadInterface.java: Line 63: public void setFlushOnCompleteRead(boolean flushOnCompleteRead); > Would it make sense, to remove this interface and to make this a property o Removed. https://asterix-gerrit.ics.uci.edu/#/c/897/4/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/comm/IChannelWriteInterface.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/comm/IChannelWriteInterface.java: Line 22: * Represents the write interface of a {@link ChannelControlBlock}. > Update comment Done https://asterix-gerrit.ics.uci.edu/#/c/897/4/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/messages/IMessageBroker.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/messages/IMessageBroker.java: Line 27: public default void registerMessagingChannel(String nodeId, IChannelControlBlock ccb) { > This method looks strange on this interface. It's not clear, why we need to Removed. https://asterix-gerrit.ics.uci.edu/#/c/897/4/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/NodeControllerService.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/NodeControllerService.java: Line 256: ncConfig.messagingPublicPort); > Can we make the creation (and availability) of this dependent on the applic Done. Now applications that do not specify messaging IP or do not provide MessagingChannelInterfaceFactory will not initialize it. https://asterix-gerrit.ics.uci.edu/#/c/897/4/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/net/MessagingNetworkManager.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/net/MessagingNetworkManager.java: Line 30: import org.apache.hyracks.api.exceptions.HyracksDataException; > Could we stick with NetException in this file? Done but I though you would suggest getting rid of the NetException completely :) Line 48: private final IMessageBroker messageBroker; > It seems that this is only needed to register a new channel, but not to rou yes, it is removed now. -- To view, visit https://asterix-gerrit.ics.uci.edu/897 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c0bd7c11c1e78954ebceff49cb274d8073a64bd Gerrit-PatchSet: 4 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Murtadha Hubail <hubail...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Murtadha Hubail <hubail...@gmail.com> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: Yingyi Bu <buyin...@gmail.com> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
