Steven Jacobs has posted comments on this change. Change subject: ASTERIXDB-1747 Implemented full lifecycle capabilities for distributed jobs ......................................................................
Patch Set 11: (16 comments) -- Can you error out a job as early as possible since prepared states are stored in CC as well? I think I'm already doing this? -- Do you need prepare a job on every node? Would it possible to only prepare it on nodes that are used? As far as I know, there is no way to know a priori where the job will run (unless I'm missing something in the ActivityClusterGraph?) In terms of tests, you probably should have unit tests (e.g., MockIto based, http://site.mockito.org/) for: -- the Works you added and the Works you dramatically changed. -- The lifecycle of things you put into the state maps, e.g., no leaks. It is still unclear for me how I can create such a test, because as far as I see a) there is no way to test at the language level, and b) there is no way to test on an individual class level (as we are looking at full lifecycle results of IHyracksClientConnection methods. Unless the suggestion is just to test functionality of the ClusterControllerService methods? Do we have such tests in Asterix already? https://asterix-gerrit.ics.uci.edu/#/c/1377/11/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/client/IHyracksClientConnection.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/client/IHyracksClientConnection.java: PS11, Line 80: specified > Start --> Distribute? Done PS11, Line 86: Exception > Specify when to stop stop? Line 88: public JobId distributeJob(JobSpecification jobSpec) throws Exception; > throws HyracksException These new methods (along with all previous methods in this class) have this because RPCInterface.Request.getResponse() is able to throw any type of Exception. PS11, Line 95: Exception > Specify when to throw this exception. These new methods (along with all previous methods in this class) have this because RPCInterface.Request.getResponse() is able to throw any type of Exception. Line 97: public JobId destroyJob(JobId jobId) throws Exception; > Fix this. These new methods (along with all previous methods in this class) have this because RPCInterface.Request.getResponse() is able to throw any type of Exception. PS11, Line 104: Exception > specify when to throw this exception. These new methods (along with all previous methods in this class) have this because RPCInterface.Request.getResponse() is able to throw any type of Exception. Line 106: public JobId startJob(JobId jobId) throws Exception; > Fix this. These new methods (along with all previous methods in this class) have this because RPCInterface.Request.getResponse() is able to throw any type of Exception. https://asterix-gerrit.ics.uci.edu/#/c/1377/11/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/executor/JobExecutor.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/executor/JobExecutor.java: PS11, Line 494: changed > What does "changed" mean here? This seems wasteful. With the previous code, the serialization was happening even if it wasn't getting used (i.e. old line 489). With this change, it's only serialized when used. https://asterix-gerrit.ics.uci.edu/#/c/1377/11/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/work/DestroyJobWork.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/work/DestroyJobWork.java: PS11, Line 46: (" > Use error code. Done PS11, Line 51: (); > Why do you need to go to every node to destroy the job? There is no way to know a priori which nodes will run the job, right? https://asterix-gerrit.ics.uci.edu/#/c/1377/11/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/work/DistributeJobWork.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/work/DistributeJobWork.java: PS11, Line 61: HyracksException > Use error code. Done PS11, Line 77: getAllNodeControllerStates > Do you need to distribute the job to every node? I don't think there is a way to know a priori which nodes will run the job, right? https://asterix-gerrit.ics.uci.edu/#/c/1377/11/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/work/JobStartWork.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/work/JobStartWork.java: PS11, Line 71: jobId > Where do you verify the jobId indeed exists? Done https://asterix-gerrit.ics.uci.edu/#/c/1377/11/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/work/DistributeJobWork.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/work/DistributeJobWork.java: Line 49: throw new RuntimeException("Trying to distribute a job that has already been distributed!"); > Use error code. AbstractWork.run() can only throw RuntimeExceptions Line 54: throw new RuntimeException(e); > Do not throw RuntimeException, but HyracksException? AbstractWork.run() can only throw RuntimeExceptions. Is there a way to fail the job here? https://asterix-gerrit.ics.uci.edu/#/c/1377/11/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/work/StartTasksWork.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/work/StartTasksWork.java: PS11, Line 192: HyracksException > Error code. Done -- To view, visit https://asterix-gerrit.ics.uci.edu/1377 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I59c3422d5c1ab7756a6a4685ac527dfe50434954 Gerrit-PatchSet: 11 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Steven Jacobs <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Carey <[email protected]> Gerrit-Reviewer: Steven Jacobs <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Xikui Wang <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
