-
Notifications
You must be signed in to change notification settings - Fork 249
exstore/pipeline integration #2978
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 17 commits
815c3a3
bb346a9
c17310c
d9a9dd2
3d08ed5
ce4cfd1
30534e1
029158d
55d9e9c
f12e8d9
cfa0550
4e09350
35eca09
4a30a20
0ed1b46
48c2cdd
466bec5
a44b6a6
4d907c8
a040feb
dc6454d
7f80874
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| import io.temporal.api.common.v1.Payloads; | ||
| import io.temporal.api.failure.v1.Failure; | ||
| import io.temporal.failure.DefaultFailureConverter; | ||
| import io.temporal.internal.payload.storage.ExternalStorageNotConfiguredException; | ||
| import io.temporal.payload.context.SerializationContext; | ||
| import java.lang.reflect.Type; | ||
| import java.util.*; | ||
|
|
@@ -71,6 +72,10 @@ public <T> T fromPayload(Payload payload, Class<T> valueClass, Type valueType) | |
| return (T) new RawValue(payload); | ||
| } | ||
|
|
||
| if (payload.getExternalPayloadsCount() > 0) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't this need to check if external storage is not configured AND there are externally stored payloads?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, it shouldn't be checking for the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yes, updated
No because if external storage is configured, it will have already walked the messages and resolved the references before getting here. If no driver can retrieve the message that error should be thrown before this point. This is just a single place to catch "external storage was not configured and we have a reference payload still". Ideally, I'd really like to remove external storage from this class but that either requires doing a bunch of eager message walking up front or adding lazy checks to a bunch of random places in the sdk which seems error prone and architecturally unsound. |
||
| throw new ExternalStorageNotConfiguredException(); | ||
| } | ||
|
|
||
| try { | ||
| String encoding = | ||
| payload.getMetadataOrThrow(EncodingKeys.METADATA_ENCODING_KEY).toString(UTF_8); | ||
|
|
||
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: I would move this to inside the if-then a few lines below and update the condition to check if
externalStorageOptionsis not null.