phet commented on code in PR #4037:
URL: https://github.com/apache/gobblin/pull/4037#discussion_r1731558718
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/DagManagerUtilsTest.java:
##########
@@ -209,6 +255,19 @@ public void
testIsDagFinishedWithFinishRunningFailureOption() throws URISyntaxEx
Assert.assertFalse(DagProcUtils.isDagFinished(dag));
}
+ private List<Dag.DagNode<JobExecutionPlan>>
randomizeNodes(Dag<JobExecutionPlan> dag) {
+ int size = dag.getNodes().size();
+ List<Dag.DagNode<JobExecutionPlan>> randomizedDagNodes = new ArrayList<>();
+
+ for (int i=0; i<size; i++) {
+ int index = rand.nextInt(size);
+ randomizedDagNodes.add(dag.getNodes().get(index));
+ }
+
+ return randomizedDagNodes;
+
+ }
Review Comment:
since this is the only place `rand` is used, suggest to make that a local
var to the method.
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/DagManagerUtilsTest.java:
##########
@@ -263,6 +333,8 @@ public static Dag<JobExecutionPlan> buildComplexDag1(String
id, long flowExecuti
return new JobExecutionPlanDagFactory().createDag(jobExecutionPlans);
}
+ // This creates a dag like this
+ // D0 -> D1 -> D2 -> D3
public static Dag<JobExecutionPlan> buildComplexDag2(String id, long
flowExecutionId,
Review Comment:
NBD, but I might call `buildLinearDagOf4Nodes`
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/proc/DagProcUtils.java:
##########
@@ -309,52 +311,43 @@ public static boolean isDagFinished(Dag<JobExecutionPlan>
dag) {
ExecutionStatus status = node.getValue().getExecutionStatus();
if (status == ExecutionStatus.FAILED || status ==
ExecutionStatus.CANCELLED) {
anyFailure = true;
- removeChildrenFromCanRun(node, dag, canRun);
+ removeDescendantsFromCanRun(node, dag, canRun);
completed.add(node);
} else if (status == ExecutionStatus.COMPLETE) {
completed.add(node);
} else if (status == ExecutionStatus.PENDING) {
// Remove PENDING node if its parents are not in canRun, this means
remove the pending nodes also from canRun set
// if its parents cannot run
- if (!areParentsInCanRun(node, canRun)) {
+ if (!areAllParentsInCanRun(node, canRun)) {
canRun.remove(node);
}
}
}
- // In the end, check if there are more nodes in canRun than completed
assert canRun.size() >= completed.size();
if (!anyFailure || failureOption ==
DagManager.FailureOption.FINISH_ALL_POSSIBLE) {
+ // In the end, check if there are more nodes in canRun than completed
return canRun.size() == completed.size();
} else if (failureOption == DagManager.FailureOption.FINISH_RUNNING) {
- //if all the remaining jobs are pending return true
+ // if all the remaining jobs are pending return true
Review Comment:
this comment doesn't seem to match...
isn't it more like
```
// if NO remaining jobs are pending, return IS finished
```
?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
