phet commented on code in PR #3938:
URL: https://github.com/apache/gobblin/pull/3938#discussion_r1596012740
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/proc/ResumeDagProcTest.java:
##########
@@ -84,16 +84,19 @@ public void tearDown() throws Exception {
@Test
public void resumeDag() throws IOException, URISyntaxException {
long flowExecutionId = System.currentTimeMillis();
Review Comment:
is there any reason NOT to just use a constant here?
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/proc/ResumeDagProc.java:
##########
@@ -82,12 +82,14 @@ protected void act(DagManagementStateStore
dagManagementStateStore, Optional<Dag
Map<String, String> jobMetadata =
TimingEventUtils.getJobMetadata(Maps.newHashMap(), node.getValue());
eventSubmitter.getTimingEvent(TimingEvent.LauncherTimings.JOB_PENDING_RESUME).stop(jobMetadata);
}
- // Set flowStartTime so that flow SLA will be based on current time
instead of original flow
+ // Set flowStartTime so that flow start deadline and flow completion
deadline will be based on current time instead of original flow
node.getValue().setFlowStartTime(flowResumeTime);
}
+ // these two statements effectively move the dag from failed dag store to
(running) dag store
Review Comment:
maybe add - "to prevent loss in the unlikely event of failure between the
two, we add first"
that said, should we check for this state around L68/69?
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/proc/ResumeDagProcTest.java:
##########
@@ -114,10 +117,8 @@ public void resumeDag() throws IOException,
URISyntaxException {
.filter(a -> a.getMethod().getName().equals("addSpec"))
.count())
.sum();
- long addedDagNodeStates =
Mockito.mockingDetails(this.dagManagementStateStore).getInvocations().stream()
- .filter(a ->
a.getMethod().getName().equals("addDagNodeState")).count();
Assert.assertEquals(resumedJobCount, expectedNumOfResumedJobs);
Review Comment:
can't we similarly use `Mockito.verify` and `Mockito.times` here rather than
`getInvocations()` and the `.filter` + `.count`?
--
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]
