Skip over entire time range if paused and batch and cache time queries #4215

samanbarghi · 2023-04-24T21:47:44Z

What changed?
Skipping over entier time range if schedule is paused. Also, batch getNextTime queries inside SideEffect and cache them.

Why?
if a schedule is paused for a very long time, when it wakes up it'll try to process the whole range of time it was paused. It will record a marker for each time. if there's many thousands, this could make the workflow task fail and the workflow get stuck. (and besides is just a waste of events.). Also, adding a Marker for getNextTime is expensive, batching and caching allows us to reduce the overhead.

How did you test it?
Unit tests

Potential risks

Is hotfix candidate?
No

dnr

I think this is good, except we need to use versioning (with tweakables) to preserve the old logic for replay

…-paused

service/worker/scheduler/workflow.go

Co-authored-by: David Reiss <[email protected]>

dnr

I think we need to invalidate the cache when Version changes also (for upgrades)

Do all the existing tests pass? This is just an internal optimization in some sense, so it doesn't necessarily need new tests, but it would be good to at least verify the marker behavior manually, if not an automated test for it

service/worker/scheduler/workflow.go

Co-authored-by: David Reiss <[email protected]>

service/worker/scheduler/workflow.go

service/worker/scheduler/workflow_test.go

Co-authored-by: David Reiss <[email protected]>

dnr

nice, the map is nice and easy.

btw, there's a typo in the PR title

service/worker/scheduler/workflow.go

service/worker/scheduler/workflow_test.go

Co-authored-by: David Reiss <[email protected]>

dnr · 2023-05-09T03:57:02Z

service/worker/scheduler/workflow_test.go

+		// add/process maxRuns schedules
+		for j := 0; j < maxRuns; j++ {
+			runStartTime := time.Date(2022, 5, i, j, 17+j%2, 0, 0, time.UTC)
+			runs = append(runs, workflowRun{


I'm glad these don't have to be in order, that'd be even more annoying

dnr · 2023-05-09T04:02:50Z

service/worker/scheduler/workflow_test.go

+	delayedCallbacks := make([]delayedCallback, backfillIterations)
+
+	expected := runIterations
+	// schedule a call back every hour to spray backfills among scheduled runs


one nitpick: can you make the backfill only start after 2022-06-01 15:00:00? i.e. I want at least one long stretch where the cache runs out and has to refill naturally. (or even better, use the value of maxNextTimeResultCacheSize.) I think it may not actually be covering that branch anymore

dnr · 2023-05-09T15:54:44Z

service/worker/scheduler/workflow.go

+			t = next.Next
+		}
+		return results
+	}).Get(&s.nextTimeResultCache))


one more thing: I think that Get into a map may actually add entries instead of resetting the map (it ends up in the data converter which does json.Unmarshal), so the map might keep growing. I think we should explicitly clear the map before Get just in case

good idea, will do.

#4215) * Skip over entier time range if paused * Refactor * versioning using tweakables * batch next time calculation in side effect * Add version and a time generator to cache time results * Update service/worker/scheduler/workflow.go Co-authored-by: David Reiss <[email protected]> * Refactor based on PR comments * Update service/worker/scheduler/workflow.go Co-authored-by: David Reiss <[email protected]> * Update service/worker/scheduler/workflow.go Co-authored-by: David Reiss <[email protected]> * Update service/worker/scheduler/workflow.go Co-authored-by: David Reiss <[email protected]> * Update service/worker/scheduler/workflow.go Co-authored-by: David Reiss <[email protected]> * Add enum for versioning * Fix a bug with skipping schedules * Additional check for Equal * Test cache * Update service/worker/scheduler/workflow.go Co-authored-by: David Reiss <[email protected]> * Use map and add backfill to test * Update service/worker/scheduler/workflow.go Co-authored-by: David Reiss <[email protected]> * Update service/worker/scheduler/workflow.go Co-authored-by: David Reiss <[email protected]> * Update service/worker/scheduler/workflow.go Co-authored-by: David Reiss <[email protected]> * Update name * Improve backfill injection and add some randomness * Clear the map before re-populating * set map to nil * update tests to refill cache --------- Co-authored-by: David Reiss <[email protected]>

samanbarghi requested a review from a team as a code owner April 24, 2023 21:47

Skip over entier time range if paused

39036c3

samanbarghi force-pushed the feature/skip-over-entinre-time-range-while-paused branch from 233ba95 to 39036c3 Compare April 24, 2023 21:48

samanbarghi requested a review from dnr April 24, 2023 21:49

Refactor

d8e6383

dnr reviewed Apr 24, 2023

View reviewed changes

samanbarghi and others added 2 commits April 27, 2023 17:27

Merge branch 'master' into feature/skip-over-entinre-time-range-while…

5782384

…-paused

versioning using tweakables

0bfb2f2

dnr approved these changes Apr 28, 2023

View reviewed changes

service/worker/scheduler/workflow.go Outdated Show resolved Hide resolved

service/worker/scheduler/workflow.go Outdated Show resolved Hide resolved

service/worker/scheduler/workflow.go Show resolved Hide resolved

samanbarghi added 2 commits April 27, 2023 17:21

batch next time calculation in side effect

13793a4

Add version and a time generator to cache time results

ee96872

dnr reviewed Apr 28, 2023

View reviewed changes

service/worker/scheduler/workflow.go Outdated Show resolved Hide resolved

service/worker/scheduler/workflow.go Outdated Show resolved Hide resolved

service/worker/scheduler/workflow.go Outdated Show resolved Hide resolved

samanbarghi and others added 2 commits April 28, 2023 12:38

Update service/worker/scheduler/workflow.go

4165936

Co-authored-by: David Reiss <[email protected]>

Refactor based on PR comments

584d190

dnr reviewed Apr 28, 2023

View reviewed changes

service/worker/scheduler/workflow.go Outdated Show resolved Hide resolved

service/worker/scheduler/workflow.go Outdated Show resolved Hide resolved

service/worker/scheduler/workflow.go Outdated Show resolved Hide resolved

service/worker/scheduler/workflow.go Outdated Show resolved Hide resolved

samanbarghi and others added 8 commits April 28, 2023 14:05

Update service/worker/scheduler/workflow.go

a861b9f

Co-authored-by: David Reiss <[email protected]>

Update service/worker/scheduler/workflow.go

46b85b7

Co-authored-by: David Reiss <[email protected]>

Update service/worker/scheduler/workflow.go

f5d4291

Co-authored-by: David Reiss <[email protected]>

Update service/worker/scheduler/workflow.go

f9eae8f

Co-authored-by: David Reiss <[email protected]>

Add enum for versioning

230541a

Fix a bug with skipping schedules

f268b46

Additional check for Equal

b4051e8

Test cache

27b66cb

dnr reviewed May 2, 2023

View reviewed changes

samanbarghi and others added 2 commits May 2, 2023 09:08

Update service/worker/scheduler/workflow.go

b7ac461

Co-authored-by: David Reiss <[email protected]>

Use map and add backfill to test

6bb79ac

samanbarghi changed the title ~~Skip over entier time range if paused~~ Skip over entier time range if paused and batch and cache time queries May 3, 2023

dnr reviewed May 3, 2023

View reviewed changes

samanbarghi changed the title ~~Skip over entier time range if paused and batch and cache time queries~~ Skip over entire time range if paused and batch and cache time queries May 3, 2023

Update service/worker/scheduler/workflow.go

732bf0f

Co-authored-by: David Reiss <[email protected]>

samanbarghi and others added 4 commits May 3, 2023 12:19

Update service/worker/scheduler/workflow.go

199ba33

Co-authored-by: David Reiss <[email protected]>

Update service/worker/scheduler/workflow.go

191d03a

Co-authored-by: David Reiss <[email protected]>

Update name

9521321

Improve backfill injection and add some randomness

6eb8d75

dnr approved these changes May 9, 2023

View reviewed changes

dnr reviewed May 9, 2023

View reviewed changes

samanbarghi added 3 commits May 9, 2023 10:21

Clear the map before re-populating

26a76df

set map to nil

790dc4b

update tests to refill cache

3fd344a

dnr approved these changes May 10, 2023

View reviewed changes

dnr added the release/1.20.3 label May 10, 2023

samanbarghi merged commit dc7f28d into temporalio:master May 10, 2023

Skip over entire time range if paused and batch and cache time queries #4215

Skip over entire time range if paused and batch and cache time queries #4215

Uh oh!

Conversation

samanbarghi commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dnr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dnr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dnr May 9, 2023

Choose a reason for hiding this comment

Uh oh!

dnr May 9, 2023

Choose a reason for hiding this comment

Uh oh!

dnr May 9, 2023

Choose a reason for hiding this comment

Uh oh!

samanbarghi May 9, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

samanbarghi commented Apr 24, 2023 •

edited

Loading