You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by "Will-Lo (via GitHub)" <gi...@apache.org> on 2023/06/21 01:01:57 UTC

[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3707: [GOBBLIN-1846] Validate Multi-active Scheduler with Logs

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: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org