Change in asterixdb[master]: Introduce MessagingNetworkManager for NC2NC AppMessaging

["Till Westmann (Code Review)" <do...@asterixdb.incubator.apache.org>]

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 <hu...@gmail.com>
Gerrit-Reviewer: Jenkins <je...@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Till Westmann <ti...@apache.org>
Gerrit-Reviewer: Yingyi Bu <bu...@gmail.com>
Gerrit-Reviewer: abdullah alamoudi <ba...@gmail.com>
Gerrit-HasComments: Yes