phet commented on code in PR #3743:
URL: https://github.com/apache/gobblin/pull/3743#discussion_r1298904021
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/FlowTriggerHandler.java:
##########
@@ -170,28 +174,80 @@ private void scheduleReminderForEvent(Properties
jobProps, MultiActiveLeaseArbit
// Add a small randomization to the minimum reminder wait time to avoid
'thundering herd' issue
String cronExpression =
createCronFromDelayPeriod(status.getMinimumLingerDurationMillis()
+ random.nextInt(schedulerMaxBackoffMillis));
- jobProps.setProperty(ConfigurationKeys.JOB_SCHEDULE_KEY, cronExpression);
- // Ensure we save the event timestamp that we're setting reminder for to
have for debugging purposes
- // in addition to the event we want to initiate
-
jobProps.setProperty(ConfigurationKeys.SCHEDULER_EVENT_TO_REVISIT_TIMESTAMP_MILLIS_KEY,
- String.valueOf(status.getEventTimeMillis()));
-
jobProps.setProperty(ConfigurationKeys.SCHEDULER_EVENT_TO_TRIGGER_TIMESTAMP_MILLIS_KEY,
- String.valueOf(originalEventTimeMillis));
- JobKey key = new JobKey(flowAction.getFlowName(),
flowAction.getFlowGroup());
- // Create a new trigger for the flow in job scheduler that is set to fire
at the minimum reminder wait time calculated
- Trigger trigger = JobScheduler.createTriggerForJob(key, jobProps,
- Optional.of(createSuffixForJobTrigger(status.getEventTimeMillis())));
+ JobKey origJobKey = new
JobKey(jobProps.getProperty(ConfigurationKeys.JOB_NAME_KEY, "<<no job name>>"),
+ jobProps.getProperty(ConfigurationKeys.JOB_GROUP_KEY, "<<no job
group>>"));
+ // Triggers:job have an N:1 relationship but the job properties must
remain constant between both, which does not
+ // allow us to keep track of additional properties for reminder events. By
reusing the same job key, we either
+ // encounter an exception that the job already exists and cannot add it to
the scheduler or have to overwrite the
+ // original job properties with the reminder event schedule. Thus, we
differentiate the job and trigger key from the
+ // original event.
+ JobKey newJobKey = new JobKey(origJobKey.getName() +
createSuffixForJobTrigger(status.getEventTimeMillis()),
+ origJobKey.getGroup());
Review Comment:
the best code is skimmable, and this is not, because there's too much to
visually parse. raise the level of abstraction, until each method call sings
out its single immediately recognizable purpose.
e.g. if we encapsulate the reaching inside of this object and that, which is
so prevalent here, it would read:
```
JobKey triggerJobKey = JobKey.fromJobProps(jobProperties);
try {
if (!this.schedulerService.getScheduler().checkExists(triggerJobKey)) {
...
}
JobDetailImpl jobDetail = updatePropsInJobDetail(triggerJobKey, status,
originalEventTimeMillis);
log.info("Flow Trigger Handler - [{}, eventTimestamp: {}] - attempting
to schedule reminder for event {}",
flowAction, originalEventTimeMillis, status.getEventTimeMillis();
JobKey reminderJobKey = calcReminderJobKey(status, triggerJobKey);
Trigger reminderTrigger =
this.schedulerService.scheduleJob(reminderJobKey, jobDetail);
log.info(...)
}
```
NOTES:
```
a. updatePropsInJobDetail - change return type to avoid cast; callee
accesses this.schedulerMaxBackoffMillis
b. leave the `getNextFireTime()` off the "about to call" logging, and only
include that in the "succeeded" log line (if in fact we really need both)
c. scheduleJob - new method that also encapsulates creating the `Trigger`;
so we can log `.getNextFireTime(), that is returned
```
up to you if you have the time RN... but this is what to aim for :)
--
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]
