[
https://issues.apache.org/jira/browse/GOBBLIN-1887?focusedWorklogId=878294&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-878294
]
ASF GitHub Bot logged work on GOBBLIN-1887:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 25/Aug/23 05:56
Start Date: 25/Aug/23 05:56
Worklog Time Spent: 10m
Work Description: phet commented on code in PR #3749:
URL: https://github.com/apache/gobblin/pull/3749#discussion_r1305155379
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/FlowTriggerHandler.java:
##########
@@ -181,36 +179,43 @@ private void scheduleReminderForEvent(Properties
jobProps, MultiActiveLeaseArbit
this.jobDoesNotExistInSchedulerCount.inc();
return;
}
- JobKey reminderJobKey = constructReminderJobKey(origJobKey,
status.getEventTimeMillis());
- JobDetailImpl jobDetail = createJobDetailForReminderEvent(origJobKey,
reminderJobKey, cronExpression,
- status.getEventTimeMillis(), originalEventTimeMillis);
- // Create a new trigger that is set to fire at the minimum reminder wait
time calculated
+ JobKey reminderJobKey = constructReminderJobKey(origJobKey, status);
+ JobDetailImpl jobDetail = createJobDetailForReminderEvent(origJobKey,
reminderJobKey, status,
+ triggerEventTimeMillis, schedulerMaxBackoffMillis);
Review Comment:
since `createJobDetailForReminderEvent` is not `static`, let's access
`this.schedulerMaxBackoffMillis` behind-the-scenes (rather than passing as an
arg)
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/FlowTriggerHandler.java:
##########
@@ -181,36 +179,43 @@ private void scheduleReminderForEvent(Properties
jobProps, MultiActiveLeaseArbit
this.jobDoesNotExistInSchedulerCount.inc();
return;
}
- JobKey reminderJobKey = constructReminderJobKey(origJobKey,
status.getEventTimeMillis());
- JobDetailImpl jobDetail = createJobDetailForReminderEvent(origJobKey,
reminderJobKey, cronExpression,
- status.getEventTimeMillis(), originalEventTimeMillis);
- // Create a new trigger that is set to fire at the minimum reminder wait
time calculated
+ JobKey reminderJobKey = constructReminderJobKey(origJobKey, status);
+ JobDetailImpl jobDetail = createJobDetailForReminderEvent(origJobKey,
reminderJobKey, status,
+ triggerEventTimeMillis, schedulerMaxBackoffMillis);
+ // Create a new trigger with a `reminder` suffix that is set to fire at
the minimum reminder wait time calculated
Trigger reminderTrigger =
JobScheduler.createTriggerForJob(reminderJobKey,
- (Properties)
jobDetail.getJobDataMap().get(GobblinServiceJobScheduler.PROPERTIES_KEY),
Optional.absent());
+ getJobPropertiesFromJobDetail(jobDetail),
Optional.of(createSuffixForJobTrigger(status)));
// TODO: remove this comment once we've confirmed this function works
- log.info("Flow Trigger Handler - [{}, eventTimestamp: {}] - attempting
to schedule reminder for event {}",
- flowAction, originalEventTimeMillis, status.getEventTimeMillis());
+ log.info("Flow Trigger Handler - [{}, eventTimestamp: {}] - attempting
to schedule reminder for event {} with"
+ + "reminderJobKey {} and reminderTriggerKey {}", flowAction,
triggerEventTimeMillis,
+ status.getEventTimeMillis(), reminderJobKey,
reminderTrigger.getKey());
this.schedulerService.getScheduler().scheduleJob(jobDetail,
reminderTrigger);
Review Comment:
any reason not to continue "all the way" and combine these four
closely-related cascading invocations that are merely sub-steps toward
accomplishing the overall goal?
```
protected Trigger createAndScheduleReminder(
JobKey origJobKey,
MultiActiveLeaseArbiter.LeasedToAnotherStatus status,
long triggerEventTimeMillis
) {
String reminderSuffix = createSuffixForJobTrigger(status);
JobKey reminderJobKey = new JobKey(origJobKey.getName() + reminderSuffix +
origJobKey.getGroup());
JobDetailImpl jobDetail = createJobDetailForReminderEvent(origJobKey,
reminderJobKey, status, triggerEventTimeMillis, this.schedulerMaxBackoffMillis);
Trigger reminderTrigger = JobScheduler.createTriggerForJob(reminderJobKey,
getJobPropertiesFromJobDetail(jobDetail),
Optional.of(reminderSuffix));
// TODO: remove this comment once we've confirmed this function works
log.info("Flow Trigger Handler - [{}, eventTimestamp: {}] - attempting to
schedule reminder for event {} with"
+ "reminderJobKey {} and reminderTriggerKey {}", flowAction,
triggerEventTimeMillis,
status.getEventTimeMillis(), reminderJobKey, reminderTrigger.getKey());
this.schedulerService.getScheduler().scheduleJob(jobDetail,
reminderTrigger);
return reminderTrigger;
}
```
(return `Trigger` for logging)
Issue Time Tracking
-------------------
Worklog Id: (was: 878294)
Remaining Estimate: 0h
Time Spent: 10m
> Increase Encapsulation in FlowTriggerHandler & Add Unit Tests
> -------------------------------------------------------------
>
> Key: GOBBLIN-1887
> URL: https://issues.apache.org/jira/browse/GOBBLIN-1887
> Project: Apache Gobblin
> Issue Type: Bug
> Components: gobblin-service
> Reporter: Urmi Mustafi
> Assignee: Abhishek Tiwari
> Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Increase abstraction and encapsulation in FlowTriggerHandler with minor
> refactoring to static helper functions. Unpack objects as late as possible to
> make {{scheduleReminderForEvent}} easier to read. Add unit tests for each of
> the static helper functions. Also ensure that the {{reminderJobKey}} is also
> used as the {{reminderTrigger's key}}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
