External Storage Integration: Lazy resolving references, general refactoring - #3016
External Storage Integration: Lazy resolving references, general refactoring#3016cconstable wants to merge 2 commits into
Conversation
3fe47fb to
831c284
Compare
| private boolean allowActivityHeartbeatDuringShutdown; | ||
| private String workerControlTaskQueue; | ||
| private PreferredVersionProvider preferredVersionProvider; | ||
| private @Nullable ExternalStorage externalStorage; |
There was a problem hiding this comment.
This doesn't have any callers in this PR but three of the PRs that build off this will use it. Included it here since those PRs are being up together.
831c284 to
460bfbf
Compare
cconstable
changed the title
cconstable
changed the title
cconstable
changed the title
| } | ||
|
|
||
| if (ExternalStorageReferences.isReference(payload)) { | ||
| throw new ExternalStorageNotConfiguredException(); |
There was a problem hiding this comment.
IIUC, this looks to be a DataConverter, which means it runs within the workflow code context. This means that this exception is handleable by user code. I think we need to move this to somewhere before the workflow code executes so we can fail the workflow task without allowing the user code to compensate.
There was a problem hiding this comment.
The context here is that if we missed integrating external storage into some piece of the SDK or a new feature was added that forgot to integrate external storage, this would catch it and throw a not configured error. It's purely a defensive fallback to make sure that SDK bugs don't appear as "your payloads can't be decoded/transformed" errors.
I do think ExternalStorageNotConfiguredException is probably the wrong error to be throwing here because it's likely that it was configured right and the sdk just isn't using external storage. Should this be a "some feature you are using has not integrated external storage. please file a bug report" type thing?
| /** | ||
| * A {@link DataConverter} that resolves external storage reference payloads before deserialization. | ||
| */ | ||
| public final class ExternalStorageResolvingDataConverter implements DataConverter { |
There was a problem hiding this comment.
There was a problem hiding this comment.
Removed this entirely in favor of just doing the lazy retrieving in the few spots we needed explicitly. After chatting this felt like too much abstraction.
| @@ -84,10 +98,21 @@ public Builder setPayloadSizeThreshold(int payloadSizeThreshold) { | |||
| return this; | |||
| } | |||
|
|
|||
| /** | |||
| * Maximum number of payload lists visited concurrently while offloading or restoring the | |||
| * payloads of a single message. Must be at least 1. Defaults to 3. | |||
There was a problem hiding this comment.
Not sure a user is going to understand what is meant by "a single message". And "a single message" isn't quite the right scope from a implementation perspective. Maybe should describe this in terms of client operations and worker tasks.
There was a problem hiding this comment.
I think this will get cleaned up in the concurrency pass. I believe (technically) a single message is correct but I agree it's not a helpful way to describe it.
| } | ||
| } | ||
|
|
||
| <T extends Message> CompletableFuture<T> store( |
There was a problem hiding this comment.
Are there legitimate places where we need to visit on the fully constructed message instead of the build (the next overload)? I presume that the caller already created a builder, constructed the message, then this would effective recreate another builder, and reconstruct the message again. Might be perf issues. I would check to see if we can drop the message overloads and only use the builder overloads to force callers into the better performing algorithm.
| } | ||
| } | ||
|
|
||
| <T extends Message> CompletableFuture<T> store( |
There was a problem hiding this comment.
This will not work for workflow task completions because the nested commands need to change the the context when they are encountered. Having an outer visitor doing that determination and then calling this method is probably okay.
| /** | ||
| * A {@link DataConverter} that resolves external storage reference payloads before deserialization. | ||
| */ | ||
| public final class ExternalStorageResolvingDataConverter implements DataConverter { |
There was a problem hiding this comment.
Maybe add a comment that this is used in non-workflow contexts.
…former to ExternalStorage, create a lazy extstore resolving data converter.
…e to match other sdks.
460bfbf to
b3804da
Compare
|
Pushed a few updates:
|
2 participants
What was changed
ExternalStorageMessageTransformertoExternalStorage.ExternalStorageResolvingDataConverter, that does the external storage work and delegates to the existing data converter for everything else.fromPayload). We don't eagerly load them.Why?
These changes support the following PRs:
Checklist