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.
if we want to double down on (group, name) then I suggest to change it
EVERYWHERE, even for `FlowStatus`
##########
gobblin-runtime/src/test/java/org/apache/gobblin/service/monitoring/FlowStatusGeneratorTest.java:
##########
@@ -58,12 +58,12 @@ public void testIsFlowRunningCompiledPastExecution() {
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);
+
when(jobStatusRetriever.getAllFlowStatusesForFlowExecutionsOrdered(flowName,
flowName)).thenReturn(
Review Comment:
dup of `flowName`
##########
gobblin-service/src/test/java/org/apache/gobblin/service/monitoring/MysqlJobStatusRetrieverTest.java:
##########
@@ -97,31 +97,31 @@ public void testGetFlowStatusFromJobStatuses() throws
Exception {
addJobStatusToStateStore(flowExecutionId, JobStatusRetriever.NA_KEY,
ExecutionStatus.COMPILED.name());
Assert.assertEquals(ExecutionStatus.COMPILED,
-
jobStatusRetriever.getFlowStatusFromJobStatuses(jobStatusRetriever.getJobStatusesForFlowExecution(FLOW_NAME,
FLOW_GROUP, flowExecutionId)));
+
JobStatusRetriever.getFlowStatusFromJobStatuses(jobStatusRetriever.getJobStatusesForFlowExecution(FLOW_NAME,
FLOW_GROUP, flowExecutionId)));
Review Comment:
again, let's reorder only if we go ALL-IN w/ this
`getJobStatusesForFlowExecution` too
##########
gobblin-runtime/src/main/java/org/apache/gobblin/service/monitoring/JobStatusRetriever.java:
##########
@@ -87,25 +87,14 @@ public abstract Iterator<JobStatus>
getJobStatusesForFlowExecution(String flowNa
* is decided by comparing {@link JobStatus#getFlowExecutionId()}.
* @return `FlowStatus`es are ordered by descending flowExecutionId.
**/
- public abstract List<FlowStatus>
getAllFlowStatusesForFlowExecutionsOrdered(String flowGroup,String flowName);
+ public abstract List<FlowStatus>
getAllFlowStatusesForFlowExecutionsOrdered(String flowGroup, String flowName);
public long getLatestExecutionIdForFlow(String flowName, String flowGroup) {
Review Comment:
this one is still in the reverse order
--
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]
