phet commented on code in PR #3707:
URL: https://github.com/apache/gobblin/pull/3707#discussion_r1237605433
##########
gobblin-runtime/src/main/java/org/apache/gobblin/scheduler/JobScheduler.java:
##########
@@ -407,6 +407,10 @@ public void scheduleJob(Properties jobProps, JobListener
jobListener, Map<String
this.scheduledJobs.put(jobName, job.getKey());
}
+ public void scheduleJobLogUtil(JobDetail job, Trigger trigger) {
Review Comment:
firstly, does this need to be `public`? I'd expect `protected`...
secondly, if the name here were read as a noun (about a util), it would be
appropriate to a class. a method should be named by a verb. reading it that
way, it suggests to do scheduling of a "JobLogUtil".
how about `logJobSchedulingTrace` or `logJobSchedulingInfo`?
or since it's so specific, `logNewlyScheduledJob`?
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/scheduler/GobblinServiceJobScheduler.java:
##########
@@ -442,6 +446,14 @@ public synchronized void scheduleJob(Properties jobProps,
JobListener jobListene
}
}
+ @Override
+ public void scheduleJobLogUtil(JobDetail job, Trigger trigger) {
+ Properties jobProps = (Properties)
job.getJobDataMap().get(JobScheduler.JOB_SCHEDULER_KEY);
+ log.info("Scheduler trigger tracing: [flowName: {} flowGroup: {}] -
nextTriggerTime: {} - Job newly scheduled",
+ jobProps.getProperty(ConfigurationKeys.FLOW_NAME_KEY, ""),
Review Comment:
let's fallback to something clearer for those reading the logs. e.g. `"<<no
group>>"` or `"<<none>>"`
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/scheduler/GobblinServiceJobScheduler.java:
##########
@@ -666,13 +687,24 @@ public static class GobblinServiceJob extends
BaseGobblinJob implements Interrup
@Override
public void executeImpl(JobExecutionContext context) throws
JobExecutionException {
- _log.info("Starting FlowSpec " + context.getJobDetail().getKey());
+ JobDetail jobDetail = context.getJobDetail();
+ _log.info("Starting FlowSpec " + jobDetail.getKey());
- JobDataMap dataMap = context.getJobDetail().getJobDataMap();
+ JobDataMap dataMap = jobDetail.getJobDataMap();
JobScheduler jobScheduler = (JobScheduler)
dataMap.get(JOB_SCHEDULER_KEY);
Properties jobProps = (Properties) dataMap.get(PROPERTIES_KEY);
JobListener jobListener = (JobListener) dataMap.get(JOB_LISTENER_KEY);
+ // Obtain trigger timestamp from trigger to pass to jobProps
+ Trigger trigger = context.getTrigger();
+ // THIS current event has already fired if this method is called, so it
now exists in <previousFireTime>
+ long triggerTimestampMillis = trigger.getPreviousFireTime().getTime();
+
jobProps.setProperty(ConfigurationKeys.SCHEDULER_EVENT_TO_TRIGGER_TIMESTAMP_MILLIS_KEY,
+ String.valueOf(triggerTimestampMillis));
+ _log.info("Scheduler trigger tracing: [flowName: {} flowGroup: {}] -
triggerTime: {} nextTriggerTime: {} - "
+ + "Job triggered by scheduler",
+ jobProps.getProperty(ConfigurationKeys.JOB_NAME_KEY),
jobProps.getProperty(ConfigurationKeys.JOB_GROUP_KEY),
+ triggerTimestampMillis, trigger.getNextFireTime().getTime());
Review Comment:
somewhat surprised to see you work again from first principles rather than
leverage the new method abstraction you just added as `scheduleJobLogUtil`...
but not a big deal (I see how the method would need to be more general for
it to work here as well)
--
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]
