Yingyi Bu has posted comments on this change. Change subject: ASTERIXDB-1747 Implemented full lifecycle capabilities for distributed jobs ......................................................................
Patch Set 23: (18 comments) https://asterix-gerrit.ics.uci.edu/#/c/1377/23//COMMIT_MSG Commit Message: PS23, Line 7: distributed distributed -> pre-distributed 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 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? 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., Distributed job %1$s cannot be found. PS23, Line 43: distributed Add job id as a parameter. 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. Create a new class, e.g., called ActiveJobDescriptor, that contains: JobId, ActivityClusterGraph, JobSepcifiction, and Set<Constraint> It looks sth. like: class ActiveJobDescriptor { JobId ...; ActivityClusterGraph ...; JobSepcifiction ...; Set<Constraint> ...; public ActiveJobStore(...){ .. } ... } The class should not have setXXX method. Then, the class of ActiveJobStore looks sth. like: class ActiveJobStore { Map<JobId, ActiveJobDescriptor> ... } PS23, Line 340: activityClusterGraphMap what if the jobId does not exist? Error out here? PS23, Line 352: jobId What if the jobId does not exist? Error out here? PS23, Line 364: activityClusterGraphConstraintsMap What if the jobId does not exist? Error out here? 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 ActiveJobStore. In the cluster controller, you will only have public ActiveJobStore getActiveJobStore() 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.serialize(acg) for every entry? I think line 497 -- 500 should be moved to line 486 and only call JavaSerializationUtils.serialize(acg); once based on pre-distributed or not. 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 PS23, Line 76: HyracksException push the null check into the getXXX method so that you do not need to check that at every caller. 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? I think it would be good to keep them as 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 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 NC. call ncs.getClusterController().notifyXXXFailure(...)? 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. PS23, Line 69: protected Why protected? I couldn't find that it is used outside of this class. -- 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
