Yingyi Bu has posted comments on this change. Change subject: ASTERIXDB-1747 Implemented full lifecycle capabilities for distributed jobs ......................................................................
Patch Set 11: (23 comments) Two high level comments: -- Can you error out a job as early as possible since prepared states are stored in CC as well? -- Do you need prepare a job on every node? Would it possible to only prepare it on nodes that are used? Detailed comments are inlined. 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. 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? PS11, Line 86: Exception Specify when to stop Line 88: public JobId distributeJob(JobSpecification jobSpec) throws Exception; > CRITICAL SonarQube violation: throws HyracksException PS11, Line 95: Exception Specify when to throw this exception. Line 97: public JobId destroyJob(JobId jobId) throws Exception; > CRITICAL SonarQube violation: Fix this. throws HyracksException PS11, Line 104: Exception specify when to throw this exception. Line 106: public JobId startJob(JobId jobId) throws Exception; > CRITICAL SonarQube violation: Fix this. Throws HyracksException https://asterix-gerrit.ics.uci.edu/#/c/1377/11/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/client/IHyracksClientInterface.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/client/IHyracksClientInterface.java: Line 41: public JobId startJob(JobId jobId) throws Exception; > CRITICAL SonarQube violation: Fix this. Line 43: public JobId distributeJob(byte[] acggfBytes) throws Exception; > CRITICAL SonarQube violation: Fix this. Line 45: public JobId destroyJob(JobId jobId) throws Exception; > CRITICAL SonarQube violation: Fix this. 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? Can you only change line 478 based on predistributed or not? In this way, you don't need to change the loop. 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/job/JobRun.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/job/JobRun.java: Line 124: this(deploymentId, jobId, EnumSet.noneOf(JobFlag.class), callback, ccs.getJobSpecification(jobId), ccs.getActivityClusterGraph(jobId)); > MAJOR SonarQube violation: format code? 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. https://cwiki.apache.org/confluence/display/ASTERIXDB/Exception+Handling PS11, Line 51: (); Why do you need to go to every node to destroy the job? E.g., the job can be only executed using a subset of nodes. 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. https://cwiki.apache.org/confluence/display/ASTERIXDB/Exception+Handling PS11, Line 77: getAllNodeControllerStates Do you need to distribute the job to every node? 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? https://asterix-gerrit.ics.uci.edu/#/c/1377/11/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/base/INodeController.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/base/INodeController.java: Line 53: public void distributeJob(JobId jobId, byte[] planBytes) throws Exception; > CRITICAL SonarQube violation: Fix this? Line 55: public void destroyJob(JobId jobId) throws Exception; > CRITICAL SonarQube violation: Fix this? 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/DestroyJobWork.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/work/DestroyJobWork.java: Line 45: throw new RuntimeException("Trying to destroy a job that was never distributed!"); > CRITICAL SonarQube violation: Use error code. https://cwiki.apache.org/confluence/display/ASTERIXDB/Exception+Handling 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!"); > CRITICAL SonarQube violation: Use error code. https://cwiki.apache.org/confluence/display/ASTERIXDB/Exception+Handling Line 54: throw new RuntimeException(e); > CRITICAL SonarQube violation: Do not throw RuntimeException, but HyracksException? Should you fail the job now, instead of throw a runtime exception? 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. https://cwiki.apache.org/confluence/display/ASTERIXDB/Exception+Handling -- 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
