Will-Lo commented on code in PR #3707:
URL: https://github.com/apache/gobblin/pull/3707#discussion_r1236131987
##########
gobblin-runtime/src/main/java/org/apache/gobblin/scheduler/JobScheduler.java:
##########
@@ -398,7 +398,8 @@ public void scheduleJob(Properties jobProps, JobListener
jobListener, Map<String
// Schedule the Quartz job with a trigger built from the job
configuration
Trigger trigger = createTriggerForJob(job.getKey(), jobProps);
this.scheduler.getScheduler().scheduleJob(job, trigger);
- LOG.info(String.format("Scheduled job %s. Next run: %s.", job.getKey(),
trigger.getNextFireTime()));
+ LOG.info("Scheduler trigger validation: [flowName: {} flowGroup: {}] -
nextTriggerTime: {} - "
+ + "Job scheduled", job.getKey().getName(), job.getKey().getGroup(),
trigger.getNextFireTime());
Review Comment:
I don't think it's guaranteed that a job here has a flowName and flowGroup
since this is the generic jobScheduler class. Also, the jobName and jobGroup is
not necessarily the flowName and flowGroup either.
##########
gobblin-runtime/src/main/java/org/apache/gobblin/scheduler/JobScheduler.java:
##########
@@ -612,7 +613,10 @@ public void executeImpl(JobExecutionContext context)
long triggerTimestampMillis = trigger.getPreviousFireTime().getTime();
jobProps.setProperty(ConfigurationKeys.SCHEDULER_EVENT_TO_TRIGGER_TIMESTAMP_MILLIS_KEY,
String.valueOf(triggerTimestampMillis));
-
+ LOG.info("Scheduler trigger validation: [flowName: {} flowGroup: {}] -
triggerTime: {} nextTriggerTime: {} - "
+ + "Job triggered by scheduler",
Review Comment:
Same comment here, the job name and job group don't map to flownames and
flowgroups exactly.
If you want to make it GaaS Specific, it has to live in the
`GobblinServiceJobScheduler`
--
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]
