Add JitterDelay option when creating workflows. #4886

ZackLK · 2022-06-29T21:42:05Z

What changed?
Added a JitterStart parameter for the customer to specify a # of seconds to jitter the start of their workflow.

Why?
When customers create a large # of workflows and specify they should all start at the same time, it can cause a thundering herd problem. This parameter allows the start time to be jittered so that the large # of workflows can be started at slightly different times.

How did you test it?
Unit test and tested manually via CLI:
./cadence --domain samples-domain workflow start --tl helloWorldGroup --wt main.helloWorldWorkflow --et 30 --input '"Jittered"' --jitter_start_seconds 5

Potential risks

Release notes

Documentation Changes

davidporter-id-au · 2022-06-30T01:07:36Z

common/util.go

+	// Add a random jitter to start time, if requested.
+	jitterStartSeconds := startRequest.GetJitterStartSeconds()
+	if jitterStartSeconds > 0 {
+		firstDecisionTaskBackoffSeconds += rand.Int31n(jitterStartSeconds + 1)


rand has a seed value somewhere?

ah, not sure that it does. Be careful to call rand.Seed(time.Now().UnixNano()) someplace lest you get a surprisingly deterministic result

Good call - turns out we already seed rand in host/service.go for every service.

davidporter-id-au

No worries with this diff as it's self-evident what's going on, but I'd normally suggest pulling the IDL changes into a separate diff/PR so as to not obscure any changes

emrahs · 2022-06-30T06:40:22Z

common/util.go

@@ -530,6 +530,12 @@ func CreateHistoryStartWorkflowRequest(
 		firstDecisionTaskBackoffSeconds += delayStartSeconds
 	}

+	// Add a random jitter to start time, if requested.


Jitter should also apply to each occurrence of a workflow when there is a cron schedule. Otherwise, only the first run of a cron workflow will benefit from the jitter.

I believe this is now working - see my latest commit.

ZackLK · 2022-07-11T21:17:28Z

Tested new cron changes via:
./cadence --domain samples-domain workflow start --tl helloWorldGroup --wt main.helloWorldWorkflow --et 30 --input '"Jittered"' --jitter_start_seconds 5 --cron '@every 10s'

(in cadence-samples) bin/helloworld -m worker

And noting that the workflow doesn't run every 10 seconds, but rather runs between 10-15 seconds every 10 seconds.

coveralls · 2022-07-11T21:30:11Z

Pull Request Test Coverage Report for Build 01820429-aee4-4cb0-bd69-9e7b43d311ab

48 of 71 (67.61%) changed or added relevant lines in 14 files are covered.
69 unchanged lines in 13 files lost coverage.
Overall coverage decreased (-0.02%) to 57.727%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tools/cli/workflowCommands.go	1	4	25.0%
common/types/shared.go	6	15	40.0%
service/frontend/workflowHandler.go	4	15	26.67%

Files with Coverage Reduction	New Missed Lines	%
common/types/shared.go	1	41.75%
service/history/execution/mutable_state_builder.go	2	69.05%
service/history/execution/mutable_state_util.go	2	36.14%
service/history/task/transfer_active_task_executor.go	2	71.43%
common/persistence/dataManagerInterfaces.go	3	58.9%
common/persistence/serialization/parser.go	4	62.41%
common/persistence/serialization/thrift_decoder.go	4	53.06%
common/types/mapper/thrift/shared.go	4	63.77%
common/task/fifoTaskScheduler.go	5	80.41%
common/persistence/sql/workflowStateMaps.go	8	81.25%

Totals
Change from base Build 0181d5e0-475a-4e8c-abb3-eb4952984e85:	-0.02%
Covered Lines:	83710
Relevant Lines:	145010

💛 - Coveralls

demirkayaender

Could you check if we have a test for next cron start? Based on the code it looks like next cron schedule should consider jitter again (expected behavior) as opposed to delay_start not being considered (expected behavior).

If this is easy to test, we should test; it can simply be breakable with a code change later.

demirkayaender · 2022-07-13T23:23:15Z

CONTRIBUTING.md

+
+If you make changes in the idls submodule and want to test them locally, you can easily do that by using go mod to use the local idls directory instead of github.com/uber/cadence-idl. Temporarily add the following to the bottom of go.mod:
+
+```replace github.com/uber/cadence-idl => ./idls```


demirkayaender · 2022-07-13T23:28:22Z

common/backoff/cron.go

-	return roundedInterval
+
+	var jitter time.Duration
+	if jitterStartSeconds > 0 {


Could be good to validate jitterStartSeconds somewhere to make sure its max value is lower than the time till next schedule

Similarly, we should return an error when it's negative.

demirkayaender · 2022-07-13T23:31:51Z

common/backoff/cron_test.go

+	{"* * * * *", "2018-12-17T08:00:00+00:00", 10, time.Second * 60},
+	{"* * * * *", "2018-12-17T08:00:10+00:00", 30, time.Second * 50},
+	{"* * * * *", "2018-12-17T08:00:25+00:00", 5, time.Second * 35},
+	{"* * * * *", "2018-12-17T08:00:59+00:00", 45, time.Second * 1},


We should try 60+ here and expect error

demirkayaender · 2022-07-13T23:33:14Z

common/backoff/cron.go

+	startTime time.Time,
+	closeTime time.Time,
+	jitterStartSeconds int32,
+) time.Duration {


if you are moving the jitter to this function, you might want to change the return type to (error, time.Duration)

demirkayaender · 2022-07-13T23:35:27Z

service/frontend/workflowHandler.go

@@ -2084,6 +2085,10 @@ func (wh *WorkflowHandler) StartWorkflowExecution(
 		return nil, wh.error(errInvalidDelayStartSeconds, scope, tags...)
 	}

+	if startRequest.GetJitterStartSeconds() < 0 {
+		return nil, wh.error(errInvalidJitterStartSeconds, scope, tags...)


demirkayaender · 2022-07-13T23:36:06Z

service/frontend/workflowHandler.go

+	if startRequest.GetJitterStartSeconds() < 0 {
+		return nil, wh.error(errInvalidJitterStartSeconds, scope, tags...)
+	}
+


We can add the jitter < cron_interval check below

…rt/no-cron.

…id confusion.

* Add JitterDelay option when creating workflows. * Update idls * getting jitter to work with cron * Update IDL Co-authored-by: David Porter <david.porter@uber.com>

Add JitterDelay option when creating workflows.

f82f4ba

ZackLK requested review from demirkayaender and a team and removed request for a team June 29, 2022 21:42

ZackLK added 2 commits June 29, 2022 14:48

Update idls

4f2ff6f

Merge branch 'master' into jitter

38e7328

davidporter-id-au reviewed Jun 30, 2022

View reviewed changes

emrahs reviewed Jun 30, 2022

View reviewed changes

ZackLK added 2 commits July 6, 2022 11:31

Merge remote-tracking branch 'upstream/master' into jitter2

33897b8

getting jitter to work with cron

0085ad6

ZackLK requested a review from emrahs July 11, 2022 21:17

Merge branch 'master' into jitter

c574282

demirkayaender reviewed Jul 13, 2022

View reviewed changes

Update IDL

c9bbd39

davidporter-id-au approved these changes Jul 15, 2022

View reviewed changes

ZackLK added 2 commits July 15, 2022 15:44

Add more JitterStart test coverage, find and fix a bug with JitterSta…

19b71e5

…rt/no-cron.

Disallow setting a JitterStart greater than the cron duration, to avo…

edfe2af

…id confusion.

ZackLK merged commit 8c449b3 into master Jul 16, 2022

ZackLK deleted the jitter branch July 16, 2022 02:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JitterDelay option when creating workflows. #4886

Add JitterDelay option when creating workflows. #4886

ZackLK commented Jun 29, 2022

davidporter-id-au Jun 30, 2022

davidporter-id-au Jun 30, 2022

ZackLK Jun 30, 2022

davidporter-id-au Jul 1, 2022

davidporter-id-au left a comment

emrahs Jun 30, 2022

ZackLK Jul 11, 2022

ZackLK commented Jul 11, 2022

coveralls commented Jul 11, 2022 •

edited

Loading

demirkayaender left a comment

demirkayaender Jul 13, 2022

demirkayaender Jul 13, 2022

demirkayaender Jul 13, 2022

demirkayaender Jul 13, 2022

demirkayaender Jul 13, 2022

demirkayaender Jul 13, 2022

demirkayaender Jul 13, 2022


		If you make changes in the idls submodule and want to test them locally, you can easily do that by using go mod to use the local idls directory instead of github.com/uber/cadence-idl. Temporarily add the following to the bottom of go.mod:

		```replace github.com/uber/cadence-idl => ./idls```

Add JitterDelay option when creating workflows. #4886

Add JitterDelay option when creating workflows. #4886

Conversation

ZackLK commented Jun 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidporter-id-au left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZackLK commented Jul 11, 2022

coveralls commented Jul 11, 2022 • edited Loading

Pull Request Test Coverage Report for Build 01820429-aee4-4cb0-bd69-9e7b43d311ab

💛 - Coveralls

demirkayaender left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 11, 2022 •

edited

Loading