Till Westmann 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 loop on an InterruptedException? 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 those classes to a better (more general) place? Line 50: //TODO make these values configurable and account for their memory usage Yes, please. 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 on the interface. Isn't this just one special message that we sent to the CC? 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 of the channel that is passed in on construction? It seems that this property should no change during the lifetime of a channel. 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 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 register one kind of channels, but not the other one. Can we remove this or move it to another (parallel) interface or ...? 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 application. I think that not every Hyracks application will need (or want) to have this service. 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? Line 48: private final IMessageBroker messageBroker; It seems that this is only needed to register a new channel, but not to route an messages. If so, it doesn't need to be a message broker and the IMessageBroker interface wouldn't need the registration API. Is that right? -- 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 <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
