Conversation
Verify supported SDKs offload oversized query results before applying the server payload limit.
| "@google-cloud/storage": "^7.0.0", | ||
| "@grpc/grpc-js": "^1.12.4", | ||
| "@protobuf-ts/protoc": "^2.8.1", | ||
| "@temporalio/activity": "^1.21.1", |
There was a problem hiding this comment.
All of these should already be updated to 1.21.1 in main.
| @@ -0,0 +1,11 @@ | |||
| { | |||
| "go": { | |||
| "minVersion": "1.48.0" | |||
There was a problem hiding this comment.
This only works for Go and not for any other language. See https://github.com/temporalio/features/blob/main/harness/go/cmd/run.go#L103. Probably either need to add harness support or do runtime skips. I'm okay with the latter on this for now. I can follow up with general config support.
| @@ -0,0 +1,11 @@ | |||
| { | |||
| "go": { | |||
| "minVersion": "1.48.0" | |||
There was a problem hiding this comment.
| "minVersion": "1.48.0" | |
| "minVersion": "v1.48.0" |
| driverName = "query-result-memory" | ||
| // Exceed the server limit so the SDK must offload the result. | ||
| resultSize = 3 * 1024 * 1024 | ||
| storageThreshold = 1024 |
There was a problem hiding this comment.
The default threshold is 256 KiB, which is much lower than the result size for each of these tests. Probably do not need to specify the threshold explicitly.
| return fmt.Errorf("query result did not use external storage") | ||
| } | ||
|
|
||
| if err := r.Client.SignalWorkflow(ctx, run.GetID(), run.GetRunID(), finishSignal, nil); err != nil { |
There was a problem hiding this comment.
Should this be done in a finally to make sure we let the workflow close if an assertion is failed earlier. Not in a defer because CheckResultDefault waits for the run result and would deadlock.
| } | ||
| return payloads, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Maybe make a func (d *memoryDriver) calls() (int, int) func so we get both values under the same lock to avoid tearing.
| if result != strings.Repeat("a", resultSize) { | ||
| return fmt.Errorf("unexpected query result") | ||
| } | ||
| if storage.storeCalls() == 0 || storage.retrieveCalls() == 0 { |
There was a problem hiding this comment.
storage is package scoped so if this is run multiple times due to variants or any future fanout or sharing across tests, these assertions won't hold. Probably need to snapshot the storage counts before executing the query, snapshot again after, and make sure the difference is what we expect (likely should be 1 for store and retrieve).
| assert driver.retrieves > 0 | ||
|
|
||
| await handle.signal(Workflow.finish) | ||
| await handle.result() |
There was a problem hiding this comment.
Doesn't the harness do this already?
| } | ||
|
|
||
| func (*memoryDriver) Type() string { | ||
| return driverName |
There was a problem hiding this comment.
Should we use a unique test driver name for features, in case these are run against cloud? This is going to be reported on worker heartbeat.
| The query returns 3 MiB, above the server's default 2 MiB error limit. The | ||
| checker verifies the value and confirms that storage and retrieval occurred. | ||
|
|
||
| Go, Python, and TypeScript currently expose functional external storage. |
There was a problem hiding this comment.
Probably can remove this as that's demonstrated by what's implemented as tests in here.
2 participants
What was changed
Verify supported SDKs offload oversized query results before applying the server payload limit.
Why?
Go had a bug, might as well write cross-SDK tests to verify this is Go only bug
Checklist
Closes
How was this tested: