External Storage Integration: Lazy resolving references, general refactoring #3016
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,144 @@ | ||
| package io.temporal.internal.payload.storage; | ||
|
|
||
| import io.temporal.api.common.v1.Payload; | ||
| import io.temporal.api.common.v1.Payloads; | ||
| import io.temporal.api.failure.v1.Failure; | ||
| import io.temporal.common.CancellationToken; | ||
| import io.temporal.common.converter.DataConverter; | ||
| import io.temporal.common.converter.DataConverterException; | ||
| import io.temporal.payload.context.SerializationContext; | ||
| import io.temporal.payload.storage.StorageDriverTargetInfo; | ||
| import java.lang.reflect.Type; | ||
| import java.util.Optional; | ||
| import javax.annotation.Nonnull; | ||
| import javax.annotation.Nullable; | ||
|
|
||
| /** | ||
| * A {@link DataConverter} that stores/retrieves payloads to/from external storage. | ||
| * | ||
| * <p>This is an internal class that is not exposed to users or workflow code. The intent is to use | ||
| * this data converter to consolidate extstore usage within the SDK. | ||
| */ | ||
| public final class ExternalStorageDataConverter implements DataConverter { | ||
|
Contributor
There was a problem hiding this comment. Need to implement |
||
|
|
||
| private final DataConverter delegate; | ||
| private final ExternalStorageRunner externalStorage; | ||
| private final @Nullable StorageDriverTargetInfo storageTarget; | ||
|
|
||
| public ExternalStorageDataConverter( | ||
| @Nonnull DataConverter delegate, @Nonnull ExternalStorageRunner externalStorage) { | ||
| this(delegate, externalStorage, null); | ||
| } | ||
|
|
||
| private ExternalStorageDataConverter( | ||
| @Nonnull DataConverter delegate, | ||
| @Nonnull ExternalStorageRunner externalStorage, | ||
| @Nullable StorageDriverTargetInfo storageTarget) { | ||
| this.delegate = delegate; | ||
| this.externalStorage = externalStorage; | ||
| this.storageTarget = storageTarget; | ||
| } | ||
|
|
||
| public ExternalStorageDataConverter withStorageTarget( | ||
| @Nullable StorageDriverTargetInfo storageTarget) { | ||
| return new ExternalStorageDataConverter(delegate, externalStorage, storageTarget); | ||
| } | ||
|
|
||
| @Override | ||
| public <T> Optional<Payload> toPayload(T value) throws DataConverterException { | ||
| Optional<Payload> converted = delegate.toPayload(value); | ||
| if (!converted.isPresent()) { | ||
| return converted; | ||
| } | ||
| Payloads stored = store(Payloads.newBuilder().addPayloads(converted.get()).build()); | ||
| return Optional.of(stored.getPayloads(0)); | ||
| } | ||
|
|
||
| @Override | ||
| public Optional<Payloads> toPayloads(Object... values) throws DataConverterException { | ||
| Optional<Payloads> converted = delegate.toPayloads(values); | ||
| if (!converted.isPresent()) { | ||
| return converted; | ||
| } | ||
| return Optional.of(store(converted.get())); | ||
| } | ||
|
|
||
| @Override | ||
| public <T> T fromPayload(Payload payload, Class<T> valueClass, Type valueType) | ||
| throws DataConverterException { | ||
| return delegate.fromPayload(retrieve(payload), valueClass, valueType); | ||
| } | ||
|
|
||
| @Override | ||
| public <T> T fromPayloads( | ||
| int index, Optional<Payloads> content, Class<T> parameterType, Type genericParameterType) | ||
| throws DataConverterException { | ||
| if (!content.isPresent() || index >= content.get().getPayloadsCount()) { | ||
| return delegate.fromPayloads(index, content, parameterType, genericParameterType); | ||
| } | ||
| Payload resolved = retrieve(content.get().getPayloads(index)); | ||
| return delegate.fromPayload(resolved, parameterType, genericParameterType); | ||
| } | ||
|
|
||
| @Override | ||
| public Object[] fromPayloads( | ||
| Optional<Payloads> content, Class<?>[] parameterTypes, Type[] genericParameterTypes) | ||
| throws DataConverterException { | ||
| if (!content.isPresent()) { | ||
| return delegate.fromPayloads(content, parameterTypes, genericParameterTypes); | ||
| } | ||
| return delegate.fromPayloads( | ||
| Optional.of(retrieveAll(content.get())), parameterTypes, genericParameterTypes); | ||
| } | ||
|
|
||
| @Override | ||
| @Nonnull | ||
| public RuntimeException failureToException(@Nonnull Failure failure) { | ||
| return delegate.failureToException(retrieveMessage(failure)); | ||
| } | ||
|
|
||
| @Override | ||
| @Nonnull | ||
| public Failure exceptionToFailure(@Nonnull Throwable throwable) { | ||
| return storeMessage(delegate.exceptionToFailure(throwable)); | ||
| } | ||
|
|
||
| @Override | ||
| @Nonnull | ||
| public DataConverter withContext(@Nonnull SerializationContext context) { | ||
| return new ExternalStorageDataConverter( | ||
| delegate.withContext(context), externalStorage, storageTarget); | ||
| } | ||
|
|
||
| private Payloads retrieveAll(Payloads payloads) { | ||
| for (Payload payload : payloads.getPayloadsList()) { | ||
| if (ExternalStorageReferences.isReference(payload)) { | ||
| return retrieveMessage(payloads); | ||
| } | ||
| } | ||
| return payloads; | ||
| } | ||
|
|
||
| private Payload retrieve(Payload payload) { | ||
| if (!ExternalStorageReferences.isReference(payload)) { | ||
| return payload; | ||
| } | ||
| return retrieveMessage(Payloads.newBuilder().addPayloads(payload).build()).getPayloads(0); | ||
| } | ||
|
|
||
| private Payloads store(Payloads payloads) { | ||
| Payloads.Builder builder = payloads.toBuilder(); | ||
| externalStorage.store(builder, storageTarget, null, CancellationToken.none()); | ||
|
Contributor
There was a problem hiding this comment. nit: I think this needs to be lifted to be an instance field so that the owner of the data converter can cancel in flight operations, or maybe provided via something like a thread local so each call site can cancel. Then again, not sure if there is anything on any of the clients that would enable cancellation. Take this as a todo rather than fixing unless others object.
Contributor
Author
There was a problem hiding this comment. Yea, AFAICT we don't have anything that could actually enable this cancellation right now but eventually, when we do, I think the token will need to be passed in with a |
||
| return builder.build(); | ||
| } | ||
|
|
||
| private <T extends com.google.protobuf.Message> T retrieveMessage(T message) { | ||
| return externalStorage.retrieve(message, CancellationToken.none()); | ||
| } | ||
|
|
||
| private Failure storeMessage(Failure failure) { | ||
| Failure.Builder builder = failure.toBuilder(); | ||
| externalStorage.store(builder, storageTarget, null, CancellationToken.none()); | ||
| return builder.build(); | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Is there a way we could possibly mark this hidden from API / doc generation for now and adjust the comment to effectively say this is no-op at this time? I know we have the other PRs as follow ups to enable it, but just in case we need to pause and release Java in between. Obviously remove all of that when it does actually get attached in some usable manner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK there isn't a way to hide this but I did add a n.b. note to the doc comment saying it's a no-op until integration is complete.