[GOBBLIN-1895] Set default trigger event time for adhoc flows orchestration in multi… #3759
Conversation
umustafi
changed the title
| // If triggerTimestampMillis is -1, then it was not set by the job trigger handler, and we cannot handle this event | ||
| if (triggerTimestampMillis == -1L) { |
There was a problem hiding this comment.
seems fine to adopt -1 as a sentinel value for "never set".
(I'm presuming we're only doing it because it would be a more far reaching rework to use Optional<Long>, which would most clearly capture the situation.)
still, let's give this constant a name to describe its meaning. e.g.TRIGGER_EVENT_TIME_NEVER_SET
| @@ -491,7 +491,7 @@ public void runJob(Properties jobProps, JobListener jobListener) throws JobExcep | |||
| try { | |||
| Spec flowSpec = this.scheduledFlowSpecs.get(jobProps.getProperty(ConfigurationKeys.JOB_NAME_KEY)); | |||
| String triggerTimestampMillis = jobProps.getProperty( | |||
| ConfigurationKeys.ORCHESTRATOR_TRIGGER_EVENT_TIME_MILLIS_KEY, "0"); | |||
| ConfigurationKeys.ORCHESTRATOR_TRIGGER_EVENT_TIME_MILLIS_KEY, "-1"); | |||
There was a problem hiding this comment.
if this should "never happen", maybe a comment just above, expressing that it should always be set... and if ever not, the orchestrator will fail the flow.
| @@ -249,8 +251,8 @@ public void orchestrate(Spec spec, Properties jobProps, long triggerTimestampMil | |||
| // If multi-active scheduler is enabled do not pass onto DagManager, otherwise scheduler forwards it directly | |||
| // Skip flow compilation as well, since we recompile after receiving event from DagActionStoreChangeMonitor later | |||
| if (flowTriggerHandler.isPresent()) { | |||
| // If triggerTimestampMillis is 0, then it was not set by the job trigger handler, and we cannot handle this event | |||
| if (triggerTimestampMillis == 0L) { | |||
| // If triggerTimestampMillis is -1, then it was not set by the job trigger handler, and we cannot handle this event | |||
There was a problem hiding this comment.
delete "-1, then it was "?
and s/and/then/
Codecov Report
@@ Coverage Diff @@
## master #3759 +/- ##
============================================
+ Coverage 47.09% 47.14% +0.05%
- Complexity 10876 10893 +17
============================================
Files 2147 2148 +1
Lines 84925 85039 +114
Branches 9420 9438 +18
============================================
+ Hits 39992 40091 +99
- Misses 41299 41307 +8
- Partials 3634 3641 +7
... and 8 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
…-active case
Dear Gobblin maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
JIRA
Description
Set trigger event time of 0 for adhoc flows orchestration in multi-active case so that adhoc flows are not skipped during orchestration at this point. If no trigger event is provided then we should skip for trigger event time of -1 (the new default)
gobblin/gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/Orchestrator.java
Line 253 in 10b6eb2
Tests
Commits