phet commented on code in PR #3995:
URL: https://github.com/apache/gobblin/pull/3995#discussion_r1669282212
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagActionReminderScheduler.java:
##########
@@ -128,23 +128,27 @@ public void execute(JobExecutionContext context) {
}
/**
- * Creates a key for the reminder job by concatenating all dagAction fields
+ * Creates a key for the reminder job by concatenating all dagAction fields
and the eventTime of the dagAction to
+ * allow reminders for actions associated with multiple flow executions
within a deadline period (e.g. another
+ * flow execution may occur before a flow finish or job start deadline
expires)
*/
- public static String createDagActionReminderKey(DagActionStore.DagAction
dagAction) {
- return String.format("%s.%s.%s.%s.%s", dagAction.getFlowGroup(),
dagAction.getFlowName(),
- dagAction.getFlowExecutionId(), dagAction.getJobName(),
dagAction.getDagActionType());
+ public static String createDagActionReminderKey(DagActionStore.LeaseParams
leaseParams) {
+ DagActionStore.DagAction dagAction = leaseParams.getDagAction();
+ return String.format("%s.%s.%s.%s.%s.%s", dagAction.getFlowGroup(),
dagAction.getFlowName(),
Review Comment:
this seems clearer:
```
String.join(".",
dagAction.getFlowGroup(),
dagAction.getFlowName(),
dagAction.getFlowExecutionId().toString(),
dagAction.getJobName(),
dagAction.getDagActionType(),
leaseParams.getEventTimeMillis().toString());
```
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagActionReminderScheduler.java:
##########
@@ -128,23 +128,27 @@ public void execute(JobExecutionContext context) {
}
/**
- * Creates a key for the reminder job by concatenating all dagAction fields
+ * Creates a key for the reminder job by concatenating all dagAction fields
and the eventTime of the dagAction to
+ * allow reminders for actions associated with multiple flow executions
within a deadline period (e.g. another
+ * flow execution may occur before a flow finish or job start deadline
expires)
Review Comment:
this doesn't really capture the nuance of why it's insufficient to
incorporate only the flowExecId into the key. explain why an event timestamp
is needed in addition to that flowExecId.
doesn't it have to do w/ event consolidation?
--
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]
