[
https://issues.apache.org/jira/browse/GOBBLIN-1887?focusedWorklogId=878464&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-878464
]
ASF GitHub Bot logged work on GOBBLIN-1887:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 25/Aug/23 22:21
Start Date: 25/Aug/23 22:21
Worklog Time Spent: 10m
Work Description: phet commented on code in PR #3749:
URL: https://github.com/apache/gobblin/pull/3749#discussion_r1306186753
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/scheduler/GobblinServiceJobScheduler.java:
##########
@@ -725,23 +725,23 @@ public void executeImpl(JobExecutionContext context)
throws JobExecutionExceptio
// Obtain trigger timestamp from trigger to pass to jobProps
Trigger trigger = context.getTrigger();
- long triggerTimestampMillis;
+ // THIS current event has already fired if this method is called, so it
now exists in <previousFireTime>
+ long triggerTimeMillis = asUTCEpochMillis(trigger.getPreviousFireTime());
// If the trigger is a reminder type event then utilize the trigger time
saved in job properties rather than the
// actual firing time
if (jobDetail.getKey().getName().contains("reminder")) {
- triggerTimestampMillis = Long.parseLong(
-
jobProps.getProperty(ConfigurationKeys.SCHEDULER_EVENT_TO_TRIGGER_TIMESTAMP_MILLIS_KEY,
"0"));
- long eventToRevisit = Long.parseLong(
-
jobProps.getProperty(ConfigurationKeys.SCHEDULER_EVENT_TO_REVISIT_TIMESTAMP_MILLIS_KEY,
"0"));
- _log.info(jobSchedulerTracePrefixBuilder(jobProps) + "triggerTime: {}
eventToRevisit: {} - Reminder job "
- + "triggered by scheduler", triggerTimestampMillis,
eventToRevisit);
+ long preservedConsensusEventTime = Long.parseLong(jobProps.getProperty(
+
ConfigurationKeys.SCHEDULER_PRESERVED_CONSENSUS_EVENT_TIME_MILLIS_KEY, "0"));
+ long expectedReminderTime = Long.parseLong(
+
jobProps.getProperty(ConfigurationKeys.SCHEDULER_EXPECTED_REMINDER_TIME_MILLIS_KEY,
"0"));
+ _log.info(jobSchedulerTracePrefixBuilder(jobProps) + "triggerTime: {}
expectedReminderTime: {} - Reminder job "
+ + "triggered by scheduler at {}", preservedConsensusEventTime,
expectedReminderTime, triggerTimeMillis);
+ // TODO: add a metric if expected reminder time far exceeds system time
} else {
- // THIS current event has already fired if this method is called, so
it now exists in <previousFireTime>
- triggerTimestampMillis =
asUTCEpochMillis(trigger.getPreviousFireTime());
-
jobProps.setProperty(ConfigurationKeys.SCHEDULER_EVENT_TO_TRIGGER_TIMESTAMP_MILLIS_KEY,
- String.valueOf(triggerTimestampMillis));
+
jobProps.setProperty(ConfigurationKeys.SCHEDULER_PRESERVED_CONSENSUS_EVENT_TIME_MILLIS_KEY,
+ String.valueOf(triggerTimeMillis));
Review Comment:
this `else` is when it's not a reminder trigger, but rather from the
original cron schedule, correct? if so, why do we set this property?
Issue Time Tracking
-------------------
Worklog Id: (was: 878464)
Time Spent: 50m (was: 40m)
> Increase Encapsulation in FlowTriggerHandler & Handle Reminder Events Properly
> ------------------------------------------------------------------------------
>
> 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: 50m
> 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.
> Bug fix 1: Ensure that the {{reminderJobKey}} is also used as the
> {{reminderTrigger's}} key
> Bug fix 2: Utilize the {{jobProps}} to determine {{triggerEventTimeMillis}}
> for reminder events
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
