External Storage: add external_storage_runner to handle core extstore logic#2094
External Storage: add external_storage_runner to handle core extstore logic#2094cconstable wants to merge 2 commits into
Conversation
cconstable
changed the title
|
|
||
| await t.throwsAsync(() => runExternalStore({ externalStorage, payloads: [makePayload(1)] }), { | ||
| instanceOf: ExternalStorageSelectorInvalidDriverError, | ||
| message: /TMPRL1109/, |
There was a problem hiding this comment.
Forgot to update these tests. Updating now.
| * @internal | ||
| * @experimental | ||
| */ | ||
| export async function runExternalStore({ |
There was a problem hiding this comment.
Could we create a class that has store and retrieve methods on it? I'm thinking ahead to where we want to add more capabilities, such as caching, and not wanting to pass more things via parameters. So that thing that are shared across all invocations (ExternalStorage object, hypothetical cache object/configuration) are provided at construction and the remaining variable pieces (payloads, target, abort signal) are parameters to store and retrieve functions.
|
|
||
| payloads: Payload[]; | ||
|
|
||
| /** Composed via `AbortSignal.any` with an internal controller so a failing driver call aborts siblings. */ |
There was a problem hiding this comment.
Let's not document implementation details, just expected behavior and usage.
| abortSignal, | ||
| }: ExternalRetrieveOptions): Promise<Payload[]> { | ||
| if (payloads.length === 0) return payloads; | ||
| if (!payloads.some(isReferencePayload)) return payloads; |
There was a problem hiding this comment.
I think we can remove this check to avoid multiple iteration of the payloads arg. And then inline the external storage check within the for loop.
| interface RetrieveItem { | ||
| index: number; | ||
| claim: StorageDriverClaim; | ||
| /** `0` for legacy references that pre-date the size_bytes field. */ |
There was a problem hiding this comment.
I think this comment is inaccurate. All externally stored payloads had set this additional data on the payload protobuf message.
| } | ||
| for (const [j, retrievedPayload] of retrieved.entries()) { | ||
| const item = group.items[j]!; | ||
| if (item.expectedSize > 0) { |
There was a problem hiding this comment.
I don't think this check is necessary and in fact might prevent retrieval of otherwise valid externally stored payloads.
| const item = group.items[j]!; | ||
| if (item.expectedSize > 0) { | ||
| const actual = payloadProtoSize(retrievedPayload); | ||
| if (actual !== item.expectedSize) { |
There was a problem hiding this comment.
This is performing extra validation that the other SDKs are not doing. Let's remove for now and add back if we feel this is necessary, but to all SDKs.
| export class CompleteAsyncError extends Error {} | ||
|
|
||
| /// ----------------------------------------------------------------------- | ||
| /// External Storage Errors (draft) |
There was a problem hiding this comment.
I'm not convinced we need specialized error types for validations that do not cross into workflow/activity user code. These mostly are carrying specific error messages and error codes and fields that are not consumed by the rest of the system. I'm not opposed to the information they carry or the error codes necessarily, just the new error types themselves if they aren't specially treated.
For errors that drivers or the driver selector may raise that we want to specifically know about, those make sense to have dedicated error types.
| @SymbolBasedInstanceOfError('ExternalStorageNotConfiguredError') | ||
| export class ExternalStorageNotConfiguredError extends Error { | ||
| constructor(message: string) { | ||
| super(`[TMPRL-SDK-00001] ${message}`); |
There was a problem hiding this comment.
I think this is https://github.com/temporalio/rules/blob/main/rules/TMPRL1105.md
2 participants
What was changed
external_storage_runnerthat handles core external storage logic.Checklist