External Storage Integration: Lazy resolving references, general refactoring - #3016
External Storage Integration: Lazy resolving references, general refactoring#3016cconstable wants to merge 1 commit 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.
…former to ExternalStorage, create a lazy extstore resolving data converter.
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.
| /** | ||
| * A {@link DataConverter} that resolves external storage reference payloads before deserialization. | ||
| */ | ||
| public final class ExternalStorageResolvingDataConverter implements DataConverter { |
There was a problem hiding this comment.
| @Nonnull | ||
| @Override | ||
| public DataConverter withContext(@Nonnull SerializationContext context) { | ||
| return new ExternalStorageResolvingDataConverter(delegate, externalStorage, context); |
There was a problem hiding this comment.
Shouldn't this call withContext on the delegate and then determine if a the outer ExternalStorageResolvingDataConverter truly needs to be reconstructed?
|
|
||
| /** | ||
| * 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.
| } | ||
| } | ||
|
|
||
| <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.
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