External Storage Integration: Lazy resolving references, general refactoring by cconstable · Pull Request #3016 · temporalio/sdk-java

cconstable · 2026-08-18T20:24:39Z

What was changed

Renames ExternalStorageMessageTransformer to ExternalStorage.
Adds a DataConverter decorator, ExternalStorageResolvingDataConverter, that does the external storage work and delegates to the existing data converter for everything else.
- This means we defer extstore operations until values are actually needed (e.g. fromPayload). We don't eagerly load them.
- This was implemented as a decorator because the existing data converter is used in the workflow sandbox (and extstore shouldn't be in there).

Why?

These changes support the following PRs:

Workflow worker integration External Storage Integration: WorkflowWorker, replay, history #3017
Nexus worker integration External Storage Integration: NexusWorker #3018
Activity worker and client integration External Storage Integration: Activity worker, client #3020
E2E extstore tests

Checklist

Added tests

cconstable · 2026-08-19T16:01:02Z

    private boolean allowActivityHeartbeatDuringShutdown;
    private String workerControlTaskQueue;
    private PreferredVersionProvider preferredVersionProvider;
+    private @Nullable ExternalStorage externalStorage;


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.

jmaeagle99 · 2026-08-20T17:33:03Z

    }

+    if (ExternalStorageReferences.isReference(payload)) {
+      throw new ExternalStorageNotConfiguredException();


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.

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?

I added a new ExternalStorageUnhandledReferenceException for when a reference payload makes it to fromPayload without being retrieved. This is a guard against SDK features that fail or fail to correctly integrate external storage. Normal misconfiguration errors should be caught at a level above fromPayload and are done so in the PRs that build off this.

jmaeagle99 · 2026-08-20T17:34:14Z

+/**
+ * A {@link DataConverter} that resolves external storage reference payloads before deserialization.
+ */
+public final class ExternalStorageResolvingDataConverter implements DataConverter {


Same concern in https://github.com/temporalio/sdk-java/pull/3016/changes#r3823884199

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.

jmaeagle99 · 2026-08-20T17:42:01Z

@@ -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.


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.

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.

jmaeagle99 · 2026-08-20T17:52:31Z

+    }
+  }
+
+  <T extends Message> CompletableFuture<T> store(


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.

jmaeagle99 · 2026-08-20T17:55:10Z

+    }
+  }
+
+  <T extends Message> CompletableFuture<T> store(


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.

Correct but workflow task completions are routed through a different store overload that includes the MessageVisitor so that the context can be changed (it's the one earlier in the file).

public <T extends Message> T storeBlocking( T message, @Nullable StorageDriverTargetInfo target, @Nullable MessageVisitor<StorageDriverTargetInfo> targetVisitor) { ... }

Regardless, it is confusing that there are so many overloads. I'll take a pass at consolidating them. I think we could just have one or two public ones. Some of the overloads above organically grew during the refactors and can be removed.

jmaeagle99 · 2026-08-21T16:10:22Z

+/**
+ * A {@link DataConverter} that resolves external storage reference payloads before deserialization.
+ */
+public final class ExternalStorageResolvingDataConverter implements DataConverter {


Maybe add a comment that this is used in non-workflow contexts.

…former to ExternalStorage, create a lazy extstore resolving data converter.

cconstable · 2026-08-21T21:35:55Z

Pushed a few updates:

I realized the names were a little off between this SDK and the Python/TS ones so I renamed ExternalStorageOptions to ExternalStorage and the previous ExternalStorage to ExternalStorageRunner (this feels a bit TS-flavored)
I moved ExternalStorage config on to the DataConverter and added a default null value for it. Matches the other SDKs.
Moved the ExternalStorageGenericWorkflowClient to the activity/client PR https://github.com/temporalio/sdk-java/pull/3020/changes. It's still awkward that some things in there eagerly fetch payloads and others just pass through but I'm not sure of a good path forward there.
I removed the lazy data converter decorator in favor of just doing the lazy external storage retrieving for describe and listWorkflowExecutions inline. Seemed like the decorator was more abstraction than necessary.

…e to match other sdks.

…h to workers. we've got the dataconverter already so we can just derive it where its needed.

…for configuring external storage.

…ce payload makes it to fromPayload without being retrieved. This is a guard against SDK features that fail or fail to correctly integrate external storage. Normal misconfiguration errors should be caught at a level above fromPayload.

jmaeagle99 · 2026-08-25T19:50:17Z

+   */
+  @Experimental
+  @Nullable
+  default ExternalStorage getExternalStorage() {


External storage should not be on DataConverter but instead on WorkflowClientOptions. The IDataConverter instance is accessible from within the workflow context, where we do not want external storage be accessible, let alone executable.

jmaeagle99 · 2026-08-25T19:52:26Z

      return (T) new RawValue(payload);
    }

+    if (ExternalStorageReferences.isReference(payload)) {


I'm not really convinced this is necessary. If we forget to plumb through retrieval somewhere, then it should likely fail at deserialization time explaining that the ExternalStorageReference message couldn't be deserialized into the target type. I also don't think we should give customers something that they can handle and attempt to compensate for in their executions.

jmaeagle99 · 2026-08-25T19:54:37Z

+  public ExternalStorageUnhandledReferenceException() {
+    super(
+        "[BUG] An external storage reference reached payload conversion without being handled. This"
+            + "is likely an SDK bug. Please file a bug report.");


nit: Concatenation bug, missing a space

jmaeagle99 · 2026-08-25T19:57:42Z

+        cancellationToken);
+  }
+
+  public <T extends Message> T retrieve(T message) {


This and retrieveAsync should passthrough a cancellation token from the caller.

jmaeagle99 · 2026-08-25T19:59:59Z

+  }
+
+  @Test
+  public void rawValueBypassesTheGuard() {


I don't understand what this test is trying to demonstrate or prove.

jmaeagle99 · 2026-08-25T20:06:40Z

+            .setSkipSearchAttributes(true)
+            .build();
+    try {
+      PayloadVisitors.visit(message.toBuilder(), options).join();


nit: Not for this PR, but I wonder if we can generate a non-mutating visitor and use that here.

jmaeagle99 · 2026-08-25T20:08:14Z

+                (context, payloads) -> {
+                  for (Payload payload : payloads) {
+                    if (ExternalStorageReferences.isReference(payload)) {
+                      CompletableFuture<List<Payload>> found = new CompletableFuture<>();


Could this just raise an exception instead of returning an exceptionally completed future so that the walk it terminated? As it is now, everything in the graph is walked even if we saw the first set of payloads are storage references; it's a waste of compute when we already know that it's going to fail.

cconstable force-pushed the extstore/foundation branch from 3fe47fb to 831c284 Compare August 18, 2026 21:03

cconstable commented Aug 19, 2026

View reviewed changes

cconstable force-pushed the extstore/foundation branch from 831c284 to 460bfbf Compare August 19, 2026 17:40

cconstable changed the title ~~refactor(extstore): general extstore refactoring. rename MessageTransformer to ExternalStorage, create a lazy extstore resolving data converter.~~ External Storage refactoring for workflow, activity, client, and nexus integration Aug 19, 2026

cconstable changed the title ~~External Storage refactoring for workflow, activity, client, and nexus integration~~ External Storage Integration: general refactoring Aug 19, 2026

cconstable changed the title ~~External Storage Integration: general refactoring~~ External Storage Integration: Lazy resolving references, general refactoring Aug 19, 2026

cconstable marked this pull request as ready for review August 19, 2026 18:57

cconstable requested a review from a team as a code owner August 19, 2026 18:57

jmaeagle99 requested changes Aug 20, 2026

View reviewed changes

jmaeagle99 reviewed Aug 21, 2026

View reviewed changes

refactor(extstore): general extstore refactoring. rename MessageTrans…

eee31a3

…former to ExternalStorage, create a lazy extstore resolving data converter.

cconstable force-pushed the extstore/foundation branch from 460bfbf to b3804da Compare August 21, 2026 20:35

refactor(extstore): move external storage to data converter and renam…

ca09b50

…e to match other sdks.

cconstable force-pushed the extstore/foundation branch from b3804da to ca09b50 Compare August 24, 2026 01:23

cconstable added 4 commits August 24, 2026 15:03

refactor(extstore): stop threading the external storage runner throug…

af36622

…h to workers. we've got the dataconverter already so we can just derive it where its needed.

Update ExternalStorageNotConfiguredException to refer to correct API …

5ebc8ee

…for configuring external storage.

lint: fix whitespace formatting issue

66f31c3

jmaeagle99 reviewed Aug 25, 2026

View reviewed changes

Uh oh!

Conversation

cconstable commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was changed

Why?

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cconstable commented Aug 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cconstable commented Aug 18, 2026 •

edited

Loading