Steven Jacobs has posted comments on this change. Change subject: ASTERIXDB-1747 Implemented full lifecycle capabilities for distributed jobs ......................................................................
Patch Set 23: (19 comments) Addressed Comments. https://asterix-gerrit.ics.uci.edu/#/c/1377/23//COMMIT_MSG Commit Message: PS23, Line 7: distributed > distributed -> pre-distributed Done https://asterix-gerrit.ics.uci.edu/#/c/1377/23/asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveJobNotificationHandler.java File asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveJobNotificationHandler.java: PS23, Line 82: "Removing the job" > "Removing the job" --> "remove job " + jobId Done https://asterix-gerrit.ics.uci.edu/#/c/1377/23/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java: PS23, Line 236: SessionConfig > The method is not used. Could it be removed? It's used within BAD when executing a query procedure. We need it in order to print results. https://asterix-gerrit.ics.uci.edu/#/c/1377/23/hyracks-fullstack/hyracks/hyracks-api/src/main/resources/errormsg/en.properties File hyracks-fullstack/hyracks/hyracks-api/src/main/resources/errormsg/en.properties: PS23, Line 42: job > Add job id as a parameter, e.g., Done PS23, Line 43: distributed > Add job id as a parameter. Done https://asterix-gerrit.ics.uci.edu/#/c/1377/23/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/ClusterControllerService.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/ClusterControllerService.java: PS23, Line 105: : private final Map<JobId, ActivityClusterGraph> activityClusterGraphMap; : : private final Map<JobId, JobSpecification> jobSpecificationMap; : : private final Map<JobId, Set<Constraint>> activityClusterGraphConstraintsMap; > Factor out the three member into a separate class, e.g., ActiveJobStore. Done PS23, Line 340: activityClusterGraphMap > what if the jobId does not exist? Error out here? Done PS23, Line 352: jobId > What if the jobId does not exist? Error out here? Done PS23, Line 364: activityClusterGraphConstraintsMap > What if the jobId does not exist? Error out here? Done PS23, Line 331: public void storeActivityClusterGraph(JobId jobId, ActivityClusterGraph acg) { : activityClusterGraphMap.put(jobId, acg); : } : : public void removeActivityClusterGraph(JobId jobId) { : activityClusterGraphMap.remove(jobId); : } : : public ActivityClusterGraph getActivityClusterGraph(JobId jobId) { : return activityClusterGraphMap.get(jobId); : } : : public void storeJobSpecification(JobId jobId, JobSpecification spec) { : jobSpecificationMap.put(jobId, spec); : } : : public void removeJobSpecification(JobId jobId) { : jobSpecificationMap.remove(jobId); : } : : public JobSpecification getJobSpecification(JobId jobId) { : return jobSpecificationMap.get(jobId); : } : : public void storeActivityClusterGraphConstraints(JobId jobId, Set<Constraint> acgConstraints) { : activityClusterGraphConstraintsMap.put(jobId, acgConstraints); : } : : public void removeActivityClusterGraphConstraints(JobId jobId) { : activityClusterGraphConstraintsMap.remove(jobId); : } : : public Set<Constraint> getActivityClusterGraphConstraints(JobId jobId) { : return activityClusterGraphConstraintsMap.get(jobId); : } > Factor out the six get/storeXXX methods to ActiveJobDescriptor and ActiveJo Done https://asterix-gerrit.ics.uci.edu/#/c/1377/23/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: PS23, Line 497: byte[] acgBytes = null; : if (!predistributed && changed) { : acgBytes = JavaSerializationUtils.serialize(acg); : } > If a job is not distributed, does it end up call JavaSerializationUtils.ser Done https://asterix-gerrit.ics.uci.edu/#/c/1377/23/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: Done https://asterix-gerrit.ics.uci.edu/#/c/1377/23/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: PS23, Line 74: entry > entry -> acg Done PS23, Line 76: HyracksException > push the null check into the getXXX method so that you do not need to check Done https://asterix-gerrit.ics.uci.edu/#/c/1377/23/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: PS23, Line 99: PartitionManager > What are the necessities to make those three fields non-final? In order to use the mockito library to monitor the method calls on this class, the creation of these had to be moved from the constructor to the start() method (since they pass "this"). This forces them to become non-final https://asterix-gerrit.ics.uci.edu/#/c/1377/23/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: PS23, Line 45: RuntimeException > Push the null check into getXXX Done https://asterix-gerrit.ics.uci.edu/#/c/1377/23/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: PS23, Line 54: RuntimeException > This shouldn't be a RuntimeException, as CC will not know this happens at a I wasn't 100% sure what you want here. I tried to implement a solution. Let me know if this is okay. https://asterix-gerrit.ics.uci.edu/#/c/1377/23/hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/integration/AbstractIntegrationTest.java File hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/integration/AbstractIntegrationTest.java: PS23, Line 66: protected > Why protected? I couldn't find that it is used outside of this class. Done PS23, Line 69: protected > Why protected? I couldn't find that it is used outside of this class. 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: 23 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Steven Jacobs <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Ildar Absalyamov <[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
