Extend Payload to include references to externally stored objects #671
Conversation
temporal/api/common/v1/message.proto
Outdated
| } | ||
|
|
||
| // Metadata describing an externally stored object that can be referenced by Payload | ||
| message ExternalPayload { |
There was a problem hiding this comment.
This isn't really an external payload, rather it's information about one. I think we should consider adding ExternalInfo (or ExternalPayload if we must) as a sub-message of Payload, the only place it's used, instead of top-level common as if commonly used in many places. But if it must remain top-level, I think it should be clear this proto message is not a form of Payload as its name implies.
There was a problem hiding this comment.
I changed to ExternalReference and made it a sub-message of Payload
temporal/api/common/v1/message.proto
Outdated
| message Payload { | ||
| map<string,bytes> metadata = 1; | ||
| bytes data = 2; | ||
| // References to data stored externally rather than embedded in the payload. |
There was a problem hiding this comment.
Not sure it is currently accurate to say these are references, it's just some info about them
There was a problem hiding this comment.
The plural external_infos is quite awkward, so I decided against it. I also considered ExternalDescriptor, but I like it slightly less than ExternalReference. Any other suggestions, which would also work for plural?
There was a problem hiding this comment.
Details?
reference makes me think it's should have a pointer to the external payload.
There was a problem hiding this comment.
If you mean the name of the field, singular ExternalDetail also reads a bit strange. Maybe Descriptor?
simvlad
force-pushed
the
simvlad/external-payloads
branch
from
4030023 to
fe9e752
Compare
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
Looks great to me. I originally was afraid of marking approved until we had done more code on server and SDK side to prove out what we want here, but I think this change is small enough that whatever we're doing with it on the server may not be necessary up front.
temporal/api/common/v1/message.proto
Outdated
| message Payload { | ||
| map<string,bytes> metadata = 1; | ||
| bytes data = 2; | ||
| // References to data stored externally rather than embedded in the payload. |
There was a problem hiding this comment.
Details?
reference makes me think it's should have a pointer to the external payload.
|
I want to note here, there is no taking this back once it's in a released version, so be sure it represents what you want |
simvlad
force-pushed
the
simvlad/external-payloads
branch
2 times, most recently
from
9cc7c2e to
3837d90
Compare
|
Changed to |
simvlad
force-pushed
the
simvlad/external-payloads
branch
from
3837d90 to
a4661ba
Compare
simvlad
force-pushed
the
simvlad/external-payloads
branch
from
a4661ba to
6aa16a0
Compare
READ BEFORE MERGING: All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted.
Extend Payload to include references to externally stored data. Currently the only information we would store is the size of the externally stored object, but it will be extended in the future once more features are added.
Payloads are limited in size, and as such require external references to pass large amounts of data. This PR extends Payload protobuf to include information (currently only size) about externally stored objects associated with the Payload. One Payload will be able to reference multiple externally stored objects in the future.
No breaking changes
This change is additive, and so won't break the server