Fix continueAsNew with large snapshot(>4MB) #482

longquanzheng · 2024-11-15T23:35:55Z

Description

Checklist

Code compiles correctly
Tests for the changes have been added
All tests passing
This PR change is backwards-compatible
This PR CONTAINS a (planned) breaking change (it is not backwards compatible)

Related Issue

Closes #397

longquanzheng · 2024-12-06T22:21:43Z

.github/workflows/ci-cadence-integ-test-disable-sticky.yml

-      - name: Dump docker logs
-        if: always()
-        uses: jwalton/gh-docker-logs@v2
+# TODO cadence sticky test is flaky


cc @lwolczynski disabled this as you saw today that it started to be flaky.
Having the sticky disabled for Temporal should be enough for protecting determinism.

longquanzheng · 2024-12-06T22:23:59Z

service/interpreter/persistence.go

+	// NOTE: using DeterministicKeys so that the JSON snapshot for continueAsNew is stable for pagination
+	// TODO: we should use DeterministicKeys for every map iteration in interpreter for safety
+	// https://github.com/indeedeng/iwf/issues/510
+	for _, k := range DeterministicKeys(am.searchAttributes) {


This is necessary otherwise the snapshot will have different order for different pages.
It wasn't a problem becase I didn't test more than one page for continueAsNew (and there wasn't prod use case running into this, luckily)

ktrops · 2024-12-06T23:04:02Z

service/interpreter/continueAsNewer.go

-			StaleSkipTimerSignals:      c.timerProcessor.Dump(),
-		}, nil
-	})
+	return c.provider.SetQueryHandler(ctx, service.ContinueAsNewDumpQueryType,


I'm having a hard time understanding how/where this pagination code (SetQueryHandler) is called when the workflow is queried. What is the code path?

better to use IDE to browse the code path:

workflowImpl.go: if input.IsResumeFromContinueAsNew { LoadInternalsFromPreviousRun(...) }

continueAsNewer.go: provider.ExecuteActivity(&resp, false, ctx, DumpWorkflowInternal...

activityImpl.go: apiClient.DefaultApi.ApiV1WorkflowInternalDumpPost(ctx)

service.go: s.client.QueryWorkflow(ctx, &pageOfSnapshot, request.GetWorkflowId(), request.GetWorkflowRunId(), service.ContinueAsNewDumpQueryType, request)

continueAsNewer.go : the registered handler of SetQueryHandler(ctx, service.ContinueAsNewDumpQueryType,

samuel27m · 2024-12-09T10:42:13Z

integ/large_data_attributes_test.go

+	assertions := assert.New(t)
+
+	// start test workflow server
+	wfHandler := signal.NewHandler()


Nit: Wouldn't it be better to create a handler class for this specific test?

I feel like using the existing is better. The test only need a workflow to do nothing, so that we can set some large DAs. The existing signal workflow is exactly like that. I would just copy the code if so.

samuel27m · 2024-12-09T10:55:10Z

service/api/handler.go

 	str, err := json.Marshal(req)
+	if len(str) > 1000 {
+		str = str[0:1000]
+	}


I thought this logging went to Datadog, is there a specific reason why we need to limit its size? I don't see a reason to limit this if it's not going to Temporal

This is for the request object. Logging will be too expensive too if the req is too big (>100 bytes).

This one is actually not going to Datadog because we have another internal handler (see handler.go in internal repo).

I wanted to limit this otherwise my local dev will look pretty bad when sending the 1MB request.

lwolczynski · 2024-12-09T16:37:03Z

service/interpreter/continueAsNewer.go

+			if request.PageSizeInBytes > 0 {
+				pageSize = request.PageSizeInBytes
+			}
+			lenInDouble := float64(len(wholeData))


Nit: no need to declare this since it's only used in line 151. I also believe they could be float32s since you are really dividing ints and just doing type conversion

I think having two lines is better for reading. and easier to debug when we want to set a breakpoint.

The compiler should be smart enough to optimize this to save a local varable.

float64 is required by the library. Golang doesn't allow int32 to divided by float like Java.

Okay, I see that Ceil requires float64, interesting... TIL

bump sleep add test rename fix static check tweak config done minor fix done fix comment fix test disable cadence test reduce test load reduce test load reduce test load fix test remove test remove test use copy

longquanzheng mentioned this pull request Nov 27, 2024

Reliability enhancement: move dump internal pagination to query method (IWF-130) #397

Closed

longquanzheng force-pushed the IWF-130 branch 2 times, most recently from dda96dd to 72271cf Compare December 6, 2024 21:43

longquanzheng commented Dec 6, 2024

View reviewed changes

longquanzheng requested review from ktrops and lwolczynski and removed request for ktrops December 6, 2024 22:24

longquanzheng marked this pull request as ready for review December 6, 2024 22:24

ktrops reviewed Dec 6, 2024

View reviewed changes

ktrops approved these changes Dec 6, 2024

View reviewed changes

longquanzheng force-pushed the IWF-130 branch from cccbf89 to 8f09ad8 Compare December 7, 2024 00:18

longquanzheng requested review from samuel27m and mixydubs December 7, 2024 05:22

longquanzheng changed the title ~~Fix continueAsNew with large snapshot~~ Fix continueAsNew with large snapshot(>4MB) Dec 7, 2024

samuel27m reviewed Dec 9, 2024

View reviewed changes

samuel27m approved these changes Dec 9, 2024

View reviewed changes

lwolczynski reviewed Dec 9, 2024

View reviewed changes

lwolczynski approved these changes Dec 9, 2024

View reviewed changes

longquanzheng added 9 commits December 9, 2024 15:45

Fix continueAsNew with large snapshot

10b9a6b

bump sleep add test rename fix static check tweak config done minor fix done fix comment fix test disable cadence test reduce test load reduce test load reduce test load fix test remove test remove test use copy

try fix cadence

e27f5d8

try fix cadence

e1cd041

cadence

2666c02

cadence

0f585d8

add back cadence test

288df02

remove

4cc8d4d

remove

a51ad7a

minor

00258f8

longquanzheng added 3 commits December 9, 2024 15:45

done

1e52a4e

revert sh

99424a5

minor improvements

5e066b5

longquanzheng force-pushed the IWF-130 branch from e9da04b to 5e066b5 Compare December 9, 2024 23:45

longquanzheng enabled auto-merge (squash) December 9, 2024 23:52

add sleep to test

a670a39

longquanzheng merged commit 1bed2dc into main Dec 10, 2024
8 checks passed

longquanzheng deleted the IWF-130 branch December 10, 2024 00:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix continueAsNew with large snapshot(>4MB) #482

Fix continueAsNew with large snapshot(>4MB) #482

longquanzheng commented Nov 15, 2024 •

edited

Loading

longquanzheng Dec 6, 2024 •

edited

Loading

longquanzheng Dec 6, 2024 •

edited

Loading

ktrops Dec 6, 2024

longquanzheng Dec 7, 2024 •

edited

Loading

samuel27m Dec 9, 2024

longquanzheng Dec 9, 2024 •

edited

Loading

samuel27m Dec 9, 2024

longquanzheng Dec 9, 2024

lwolczynski Dec 9, 2024

longquanzheng Dec 9, 2024 •

edited

Loading

lwolczynski Dec 10, 2024

Fix continueAsNew with large snapshot(>4MB) #482

Fix continueAsNew with large snapshot(>4MB) #482

Conversation

longquanzheng commented Nov 15, 2024 • edited Loading

Description

Checklist

Related Issue

longquanzheng Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

longquanzheng Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

ktrops Dec 6, 2024

Choose a reason for hiding this comment

longquanzheng Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

samuel27m Dec 9, 2024

Choose a reason for hiding this comment

longquanzheng Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

samuel27m Dec 9, 2024

Choose a reason for hiding this comment

longquanzheng Dec 9, 2024

Choose a reason for hiding this comment

lwolczynski Dec 9, 2024

Choose a reason for hiding this comment

longquanzheng Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

lwolczynski Dec 10, 2024

Choose a reason for hiding this comment

longquanzheng commented Nov 15, 2024 •

edited

Loading

longquanzheng Dec 6, 2024 •

edited

Loading

longquanzheng Dec 6, 2024 •

edited

Loading

longquanzheng Dec 7, 2024 •

edited

Loading

longquanzheng Dec 9, 2024 •

edited

Loading

longquanzheng Dec 9, 2024 •

edited

Loading