External Storage: foundational types and config#2092
Conversation
| * | ||
| * @experimental | ||
| */ | ||
| export const DEFAULT_PAYLOAD_SIZE_THRESHOLD = 256 * 1024; |
There was a problem hiding this comment.
@jmaeagle99 I think this could be helpful for users of the SDK and it's used in the tests as well. Are there any big drawbacks to leaving this exported?
There was a problem hiding this comment.
It being used in our tests shouldn't necessarily mean that we make it a publicly exposed and documented value. It is important for our testing because we want to make sure that system exhibits different behaviors above/below the threshold value, but I'm not convinced that customers would be testing that.
What customer scenario does exposing this value actually enable?
There was a problem hiding this comment.
Agreed it shouldn't just be exposed for tests but I could imagine folks deciding to use Temporal sanctioned defaults for things like this and then writing some extensions/tooling that reference it. It is more API surface area though.
My questions were "is this value important for users to know to understand how this feature works?" and "can I imagine someone hard-coding this number somewhere?".
I don't feel too strongly either way.
cconstable
changed the title
| */ | ||
| export type StorageDriverTarget = StorageDriverWorkflowInfo | StorageDriverActivityInfo; |
There was a problem hiding this comment.
nit: We called this StorageDriverTargetInfo in Go and I don't think Python has a named sum type for this. I would make it consistent with either one of those.
There was a problem hiding this comment.
I'll go with StorageDriverTargetInfo. I do think the prefix might be odd in some languages.
| /** | ||
| * User-supplied function that picks the destination driver for a given | ||
| * payload, or returns `null` to keep the payload inline. Required when | ||
| * more than one driver is registered. |
There was a problem hiding this comment.
Required when more than one driver is registered.
nit: This is a behavior of ExternalStorage and not of the selector itself. I would make the comment there rather than here, if it isn't already there.
| * @internal | ||
| * @experimental | ||
| */ | ||
| export function storageTargetFromContext(ctx: SerializationContext | undefined): StorageDriverTarget | undefined { |
There was a problem hiding this comment.
SerializationContext is only available/popoulated under symmetrical access to the underlying information (where the information is available on both the caller and the callee side). I would not tie StorageDriverTarget specifically to SerializationContext since it doesn't have that symmetry requirement; it is best effort of providing as much information as possible and is only use on the caller/storing side.
| * | ||
| * @experimental | ||
| */ | ||
| export const DEFAULT_PAYLOAD_SIZE_THRESHOLD = 256 * 1024; |
There was a problem hiding this comment.
It being used in our tests shouldn't necessarily mean that we make it a publicly exposed and documented value. It is important for our testing because we want to make sure that system exhibits different behaviors above/below the threshold value, but I'm not convinced that customers would be testing that.
What customer scenario does exposing this value actually enable?
| } | ||
|
|
||
| this.drivers = [...drivers]; | ||
| this.driverSelector = driverSelector; |
There was a problem hiding this comment.
Consider making an artificial driver selector if driver count is 1 and no selector was specified. Then observers of the ExternalStorage type don't have to reason about whether a selector is used or not and how they interpret the driver list when there is no selector.
|
|
||
| const readerConv = makeConverter(undefined); | ||
| await t.throwsAsync(() => decodeArrayFromPayloads(readerConv, storedPayloads!), { | ||
| instanceOf: Error, |
There was a problem hiding this comment.
Either the test should describe more generically what it's testing (e.g. remove TMPRL1105 from test title) or it should check for the specific error code or error type.
| t.deepEqual(decoded, ['hello', { v: 42 }]); | ||
| }); | ||
|
|
||
| test('encodeToPayloads is a no-op when externalStorage is undefined', async (t) => { |
There was a problem hiding this comment.
Can you add that isReferencePayload returns false?
| const details = failure.applicationFailureInfo?.details?.payloads ?? []; | ||
| t.is(details.length, 1); | ||
| t.truthy(details[0]!.externalPayloads?.length); | ||
|
|
There was a problem hiding this comment.
Can you add that isReferencePayload returns true?
| t.is(loaded.externalStorage, externalStorage); | ||
| }); | ||
|
|
||
| // Silence the unused-imports warning for converter types we keep around |
There was a problem hiding this comment.
artifact of lint warnings I was having. removing.
| retrieveCalls: [], | ||
| async store(context, payloads) { | ||
| driver.storeCalls.push({ context, payloads }); | ||
| if (opts.onStore) return opts.onStore(payloads); |
There was a problem hiding this comment.
I feel that we should do one of the following:
- Don't let onStore and onRetrieve return anything and just use it for error simulation
- Split this driver into two: one that performs in-memory storage and one that allows for generic callbacks. Only one of them would need to support error simulation.
2 participants
What was changed
extstore.ts.temporal/api/sdk/v1/external_storage.prototypes.externalStorage?: ExternalStorage;field toDataConverterfor exposing External Storage config.Why?
Part of Large Payloads work.
Checklist
New tests were added. Linter passes.