asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Murtadha Hubail (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in hyracks[master]: Add Global Resource Id Factory
Date Tue, 01 Dec 2015 13:24:32 GMT
Murtadha Hubail has posted comments on this change.

Change subject: Add Global Resource Id Factory
......................................................................


Patch Set 3:

(8 comments)

https://asterix-gerrit.ics.uci.edu/#/c/485/3/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/application/ICCApplicationEntryPoint.java
File hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/application/ICCApplicationEntryPoint.java:

Line 28:     public boolean isReadyToProcessUnqiueIdRequests();
> Does this need to be on the ApplicationEntryPoint? Would the ApplicationCon
The thing is this logic needs to be application specific. Asterix is using the same CC and
NC application context from Hyracks. The only application specific implementations in Asterix
are CC/NCApplicationEntryPoint. If we need to implement a customized CC and NC application
contexts for Asterix, it is going to require many changes. Do you think that we need to provide
these implementations for this change?


https://asterix-gerrit.ics.uci.edu/#/c/485/3/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/application/INCApplicationEntryPoint.java
File hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/application/INCApplicationEntryPoint.java:

Line 32:     public long getNCMaxUniqueId() throws Exception;
> This doesn't seem to fit in very well with the lifecycle-oriented methods o
Again, application specific implementation in NCApplicationEntryPoint.


https://asterix-gerrit.ics.uci.edu/#/c/485/3/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/context/IHyracksRootContext.java
File hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/context/IHyracksRootContext.java:

Line 32: 
> Does this need to be part of the RootContext? That seems very high up in th
This request needs to go to NodeControllerService to send it to the CC. IHyracksRootContext
is the bridge between INCApplicationContext and NodeControllerService. We can move this to
INCApplicationContext by changing the current hierarchy and passing NodeControllerService
reference to INCApplicationContext. Thoughts?


https://asterix-gerrit.ics.uci.edu/#/c/485/3/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/base/IClusterController.java
File hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/base/IClusterController.java:

Line 75:     public void getUnqiueId(InetSocketAddress ncAddress, String nodeId) throws Exception;
> I think that we shouldn't need the ncAddress here? Isn't the nodeId enough 
This is true if we assume that no node will send such request before registration. In the
current implementation, a node is able to submit such request before its registration. I have
no problem removing it, but in case of such invalid request, we won't have the node ip to
respond by an exception.


https://asterix-gerrit.ics.uci.edu/#/c/485/3/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/ipc/CCNCFunctions.java
File hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/ipc/CCNCFunctions.java:

Line 1265:         private final InetSocketAddress ncAddress;
> Again, I think the we can do without this address.
See comment in IClusterController.


https://asterix-gerrit.ics.uci.edu/#/c/485/3/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/ipc/ClusterControllerRemoteProxy.java
File hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/ipc/ClusterControllerRemoteProxy.java:

Line 158:     public void getUnqiueId(InetSocketAddress ncAddress, String nodeId) throws Exception
{
> Again, I think the we can do without this address.
See comment in IClusterController.


https://asterix-gerrit.ics.uci.edu/#/c/485/3/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/NodeControllerService.java
File hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/NodeControllerService.java:

Line 165:     private final LinkedBlockingQueue<GetUniqueIdResponseFunction> uniqueIdResponseQ;
> Why do we need a new queue here?
The existing queue is used to collect requests and process them sequentially. This queue is
used for responses (unique id responses in specific). Since multiple concurrent threads may
request unique ids, this queue is used to synchronize them.


https://asterix-gerrit.ics.uci.edu/#/c/485/3/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/file/GlobalResourceIdFactory.java
File hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/file/GlobalResourceIdFactory.java:

Line 28:     private final IHyracksRootContext rootCtx;
> Again, this might not need to be the root context.
See comment in IHyracksRootContext.


-- 
To view, visit https://asterix-gerrit.ics.uci.edu/485
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9f49234eebe912c48e7f71980433a9b42595741
Gerrit-PatchSet: 3
Gerrit-Project: hyracks
Gerrit-Branch: master
Gerrit-Owner: Murtadha Hubail <hubailmor@gmail.com>
Gerrit-Reviewer: Ian Maxon <imaxon@apache.org>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Murtadha Hubail <hubailmor@gmail.com>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message