phet commented on code in PR #4040:
URL: https://github.com/apache/gobblin/pull/4040#discussion_r1735455100
##########
gobblin-runtime/src/test/java/org/apache/gobblin/service/monitoring/FlowStatusGeneratorTest.java:
##########
@@ -77,12 +77,12 @@ public void skipFlowConcurrentCheckSameFlowExecutionId() {
JobStatus jobStatus =
JobStatus.builder().flowGroup(flowGroup).flowName(flowName).flowExecutionId(flowExecutionId)
.jobName(JobStatusRetriever.NA_KEY).jobGroup(JobStatusRetriever.NA_KEY).eventName(ExecutionStatus.COMPILED.name()).build();
Iterator<JobStatus> jobStatusIterator =
Lists.newArrayList(jobStatus).iterator();
- FlowStatus flowStatus = new
FlowStatus(flowName,flowGroup,flowExecutionId,jobStatusIterator,ExecutionStatus.COMPILED);
-
when(jobStatusRetriever.getAllFlowStatusesForFlowExecutionsOrdered(flowName,
flowGroup)).thenReturn(
+ FlowStatus flowStatus = new FlowStatus(flowName, flowGroup,
flowExecutionId, jobStatusIterator,ExecutionStatus.COMPILED);
Review Comment:
it's confusing that `org.apache.gobblin.service.monitoring.FlowStatus` uses
the (name, group) ordering. personally, I SO MUCH prefer (group, name), but
the fact that some of these other long-standing classes don't follow it, makes
me question whether we ought to just retreat and accept the (name, group)
ordering as a fact of life.
I'm NOT against the change, just recognizing it would take more work and
wanting to ensure we have a consistent end-state. so if we want to double down
on (group, name) then let's change it EVERYWHERE, even for `FlowStatus` (and
the other methods of `JobStatusRetriever`
--
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]
