Add initial extstore types#2900
Conversation
| /** Configuration for offloading large payloads to external storage. */ | ||
| @Experimental | ||
| public final class ExternalStorage { | ||
| private static final int DEFAULT_PAYLOAD_SIZE_THRESHOLD = 256 * 1024; |
There was a problem hiding this comment.
I kept this private to be consistent with TS. However, I had to remove DEFAULT_PAYLOAD_SIZE_THRESHOLD links from the doc comments so they would actually resolved (and not be broken). Seems reasonable to let users knows what the default is in the doc comments so I added a "Defaults to 256 KiB" but this feels a little worse to me than just making this public. Looking for feedback.
There was a problem hiding this comment.
Could we make this package-private (so just remove the private keyword) and then generate the docs via javadoc with -package? Haven't tried it myself and not sure what else might become docuemented.
There was a problem hiding this comment.
I think promoting to package is reasonable. One consideration for making it public is being able to reference it in generated docs e.g. on the setThreshold method "The default value is {@link ExternalStorage.DEFAULT_PAYLOAD_SIZE_THRESHOLD" instead of what I have now: "Defaults to 256 KiB".
There was a problem hiding this comment.
I think you can use @value to embed the actual value. I still don't see the value that having an effective public constant has for customers. Sure, it's great they know what the default threshold is, but they aren't going to write code specifically with that in mind. So we shouldn't provide it in code. Documentation, absolutely.
| return this; | ||
| } | ||
|
|
||
| public ExternalStorage build() { |
There was a problem hiding this comment.
Right now, these all throw state exceptions (which are tested). In a subsequent PR, I would like to replace these with stable diagnostic codes.
There was a problem hiding this comment.
For these in particular, do we need diagnostic codes? The messages are fairly clear as to what the issue is and prevent the use of external storage at startup rather than having to report during execution runtime.
There was a problem hiding this comment.
I am a proponent of best-effort, stable diagnostic codes if the mechanism for adding them very easy. I have a little write-up about this that I wanted to share but the tl;dr is I think we should make a best effort to tag anything that's actionable if for nothing else it supports automated tooling (e.g. AI) and feels like a healthy industry trend (hand wavy gesture). I would not have felt this way 4 years ago.
…s could be unstable.
|
|
||
| /** Context passed to {@link StorageDriver#retrieve}. */ | ||
| @Experimental | ||
| public final class StorageDriverRetrieveContext {} |
There was a problem hiding this comment.
Thinking about if making this and StorageDriverStoreContext interfaces instead of finalized classes. Might make it easier for us internally define the implementations and thus refactor/update them as necessary. In either case, there is burden on driver authors who write unit tests for their drivers to have to adapt to either changing APIs on classes or interfaces, but interfaces would allow them to implement however they deem necessary to fulfill the context contract.
| private Builder() {} | ||
|
|
||
| /** At least one driver is required. When more than one is set, a selector is also required. */ | ||
| public Builder setDrivers(@Nonnull List<StorageDriver> drivers) { |
There was a problem hiding this comment.
Should we document that when calling setDrivers or setDriver that the last call wins? Not sure if that is clear, especially in the case of calling setDriver multiple times.
There was a problem hiding this comment.
That's a good builder pattern call out 😄 I'll update.
| } | ||
|
|
||
| @Test(expected = IllegalStateException.class) | ||
| public void multipleDriversRequireSelector() { |
There was a problem hiding this comment.
Maybe some more tests validating that calling setDriver and setDrivers in various orders only allows the last call to win.
jmaeagle99
left a comment
jmaeagle99
left a comment
There was a problem hiding this comment.
Left some comments but overall looks good. Please hold off until a Java owner reviews and approves.
2 participants
What was changed
Adds the
@ExperimentalExternal Storage API surface underio.temporal.payload.storage. Typedefinitions only.
Types:
StorageDriver: driver contract (asyncstore/retrieve,getName/getType).StorageDriverClaim: claim-check token for an externally stored payload.StorageDriverSelector: chooses a driver per payload, or keeps it inline.StorageDriverStoreContext/StorageDriverRetrieveContext: driver-call contexts.StorageDriverTargetInfoplusStorageDriverWorkflowInfo/StorageDriverActivityInfo: best-effort,store-side target identity.
ExternalStorage(plusBuilder): config for drivers, optional selector, and size threshold.Notes:
WorkflowClientOptionssetter yet.IllegalStateException. Stable diagnostic codes are a follow-up.Why?
Checklist