Added start time for supporting restarts for fixed rate schedules #476

pmahindrakar-oss · 2022-09-15T14:23:36Z

Signed-off-by: Prafulla Mahindrakar [email protected]

TL;DR

Updated the cron library to accept startTime for timedJob
Using the lastExecution time when starting the fixed rate schedules

Testing Done:

Registered the fixed rate schedule for 1 min rate
Brought down the scheduler for a few hours
Verified the catch up routine ran and caught up to current time using the last Execution timestamp
Also verified the new routines are also respecting the last execution time

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

Remove the 'fixes' keyword if there will be multiple PRs to fix the linked issue

fixes flyteorg/flyte#2885

Follow-up issue

katrogan · 2022-09-15T18:24:59Z

can you add a tracking issue?

katrogan

btw is this ready for review?

pmahindrakar-oss · 2022-09-16T13:35:59Z

@katrogan yes it ready for review

codecov · 2022-09-16T13:44:15Z

Codecov Report

Merging #476 (64b34b4) into master (00915ec) will increase coverage by 1.29%.
The diff coverage is n/a.

❗ Current head 64b34b4 differs from pull request most recent head 2c215a1. Consider uploading reports for the commit 2c215a1 to get more accurate results

@@            Coverage Diff             @@
##           master     #476      +/-   ##
==========================================
+ Coverage   58.64%   59.93%   +1.29%     
==========================================
  Files         172      168       -4     
  Lines       16365    13171    -3194     
==========================================
- Hits         9597     7894    -1703     
+ Misses       5920     4440    -1480     
+ Partials      848      837      -11

Flag	Coverage Δ
unittests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

see 159 files with indirect coverage changes

scheduler/core/gocron_scheduler_test.go

Signed-off-by: Prafulla Mahindrakar <[email protected]>

Signed-off-by: pmahindrakar-oss <[email protected]>

katrogan · 2022-11-09T20:53:33Z

scheduler/core/gocron_scheduler.go

@@ -271,7 +271,11 @@ func (g *GoCronScheduler) AddFixedIntervalJob(ctx context.Context, job *GoCronJo
 	var jobFunc cron.TimedFuncJob
 	jobFunc = job.Run

-	entryID := g.cron.ScheduleTimedJob(cron.ConstantDelaySchedule{Delay: d}, jobFunc)
+	var lastExecTime time.Time
+	if job.lastTime != nil {


nit: should this field name change to job.lastExecTime too?

Changing this would mean maintaining backward compat. I will clean this up when i next make change to this file

On second thought, the JobStore will get loaded again after new version, so it will start using the new field after the restart, so we don't have to worry about backward compat.

Awesome, can you verify that there are indeed no problems on restart locally?

katrogan · 2022-11-09T20:58:08Z

tests/scheduler_test.go

+		}
+	})
+
+	t.Run("without schedule time", func(t *testing.T) {


sorry if I'm being dense - can you point out how this test differs than the one above?

check L62 & L90,
The last Parameter to schedulerJob is different.
And also the assertion in timedFuncWithSchedule

Refactored the test to remove redundant code which makes this test clearer

Signed-off-by: pmahindrakar-oss <[email protected]>

Signed-off-by: eduardo apolinario <[email protected]>

…yteorg#476) * Added race skip check Signed-off-by: Prafulla Mahindrakar <[email protected]> * lint Signed-off-by: Prafulla Mahindrakar <[email protected]> * Fixed unit tests Signed-off-by: pmahindrakar-oss <[email protected]> * Moved to integration test Signed-off-by: pmahindrakar-oss <[email protected]> * refactored integration test Signed-off-by: pmahindrakar-oss <[email protected]> * nit : rename to lastTime Signed-off-by: pmahindrakar-oss <[email protected]> * nit : revert Signed-off-by: pmahindrakar-oss <[email protected]> * lastTime -> lastExecTime Signed-off-by: pmahindrakar-oss <[email protected]> * integration test tag Signed-off-by: pmahindrakar-oss <[email protected]> --------- Signed-off-by: Prafulla Mahindrakar <[email protected]> Signed-off-by: pmahindrakar-oss <[email protected]> Signed-off-by: eduardo apolinario <[email protected]> Co-authored-by: eduardo apolinario <[email protected]> Signed-off-by: TungHoang <[email protected]>

* Added race skip check Signed-off-by: Prafulla Mahindrakar <[email protected]> * lint Signed-off-by: Prafulla Mahindrakar <[email protected]> * Fixed unit tests Signed-off-by: pmahindrakar-oss <[email protected]> * Moved to integration test Signed-off-by: pmahindrakar-oss <[email protected]> * refactored integration test Signed-off-by: pmahindrakar-oss <[email protected]> * nit : rename to lastTime Signed-off-by: pmahindrakar-oss <[email protected]> * nit : revert Signed-off-by: pmahindrakar-oss <[email protected]> * lastTime -> lastExecTime Signed-off-by: pmahindrakar-oss <[email protected]> * integration test tag Signed-off-by: pmahindrakar-oss <[email protected]> --------- Signed-off-by: Prafulla Mahindrakar <[email protected]> Signed-off-by: pmahindrakar-oss <[email protected]> Signed-off-by: eduardo apolinario <[email protected]> Co-authored-by: eduardo apolinario <[email protected]>

pmahindrakar-oss requested review from katrogan, wild-endeavor, anandswaminathan, EngHabu and kumare3 as code owners September 15, 2022 14:23

katrogan reviewed Sep 15, 2022

View reviewed changes

pmahindrakar-oss force-pushed the fixedrate-issue branch from 7c91a91 to 6dcbad5 Compare September 16, 2022 13:35

pmahindrakar-oss requested a review from katrogan September 16, 2022 13:36

katrogan reviewed Sep 16, 2022

View reviewed changes

scheduler/core/gocron_scheduler_test.go Outdated Show resolved Hide resolved

pmahindrakar-oss requested a review from katrogan September 19, 2022 10:09

pmahindrakar-oss added 3 commits November 8, 2022 23:09

Added race skip check

9d9f407

Signed-off-by: Prafulla Mahindrakar <[email protected]>

lint

8199f26

Signed-off-by: Prafulla Mahindrakar <[email protected]>

Fixed unit tests

3db1d5f

Signed-off-by: pmahindrakar-oss <[email protected]>

pmahindrakar-oss force-pushed the fixedrate-issue branch from 305978d to 3db1d5f Compare November 9, 2022 05:58

Moved to integration test

62b7d74

Signed-off-by: pmahindrakar-oss <[email protected]>

katrogan previously approved these changes Nov 9, 2022

View reviewed changes

refactored integration test

1583001

Signed-off-by: pmahindrakar-oss <[email protected]>

pmahindrakar-oss dismissed katrogan’s stale review via 1583001 November 10, 2022 07:45

pmahindrakar-oss added 2 commits November 10, 2022 13:47

nit : rename to lastTime

045085f

Signed-off-by: pmahindrakar-oss <[email protected]>

nit : revert

57ef179

Signed-off-by: pmahindrakar-oss <[email protected]>

pmahindrakar-oss requested a review from katrogan November 10, 2022 08:41

pmahindrakar-oss added 2 commits November 10, 2022 14:18

lastTime -> lastExecTime

a1b7913

Signed-off-by: pmahindrakar-oss <[email protected]>

integration test tag

b06039b

Signed-off-by: pmahindrakar-oss <[email protected]>

katrogan previously approved these changes Nov 10, 2022

View reviewed changes

Merge remote-tracking branch 'origin' into fixedrate-issue

2c215a1

Signed-off-by: eduardo apolinario <[email protected]>

eapolinario dismissed katrogan’s stale review via 2c215a1 May 8, 2023 18:09

eapolinario requested a review from hamersaw as a code owner May 8, 2023 18:09

eapolinario self-requested a review as a code owner May 8, 2023 18:09

eapolinario approved these changes May 8, 2023

View reviewed changes

eapolinario merged commit 507ad44 into master May 8, 2023

eapolinario deleted the fixedrate-issue branch May 8, 2023 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added start time for supporting restarts for fixed rate schedules #476

Added start time for supporting restarts for fixed rate schedules #476

pmahindrakar-oss commented Sep 15, 2022 •

edited

Loading

katrogan commented Sep 15, 2022

katrogan left a comment

pmahindrakar-oss commented Sep 16, 2022

codecov bot commented Sep 16, 2022 •

edited

Loading

katrogan Nov 9, 2022

pmahindrakar-oss Nov 10, 2022

pmahindrakar-oss Nov 10, 2022

katrogan Nov 10, 2022

katrogan Nov 9, 2022

pmahindrakar-oss Nov 10, 2022 •

edited

Loading

pmahindrakar-oss Nov 10, 2022

Added start time for supporting restarts for fixed rate schedules #476

Added start time for supporting restarts for fixed rate schedules #476

Conversation

pmahindrakar-oss commented Sep 15, 2022 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

katrogan commented Sep 15, 2022

katrogan left a comment

Choose a reason for hiding this comment

pmahindrakar-oss commented Sep 16, 2022

codecov bot commented Sep 16, 2022 • edited Loading

Codecov Report

katrogan Nov 9, 2022

Choose a reason for hiding this comment

pmahindrakar-oss Nov 10, 2022

Choose a reason for hiding this comment

pmahindrakar-oss Nov 10, 2022

Choose a reason for hiding this comment

katrogan Nov 10, 2022

Choose a reason for hiding this comment

katrogan Nov 9, 2022

Choose a reason for hiding this comment

pmahindrakar-oss Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

pmahindrakar-oss Nov 10, 2022

Choose a reason for hiding this comment

pmahindrakar-oss commented Sep 15, 2022 •

edited

Loading

codecov bot commented Sep 16, 2022 •

edited

Loading

pmahindrakar-oss Nov 10, 2022 •

edited

Loading