Rename PayloadEncoder -> PayloadCodec. #749
Conversation
Adjust interface to take a Payload slice.
robholland
requested review from
mfateev,
Sushisource,
alexshtin,
Spikhalskiy,
bergundy,
cretz and
mmcshane
as code owners
| ) | ||
| require.NoError(t, err) | ||
| compUnderMinPayload, err := zlibIgnoreMinConv.ToPayload(data) | ||
| require.NoError(t, err) | ||
| require.True(t, proto.Equal(uncompPayload, compUnderMinPayload)) | ||
| } | ||
|
|
||
| func TestEncodingDataConverterClone(t *testing.T) { |
There was a problem hiding this comment.
I have removed the cloning from ToPayload(s) because I believe the clone is only required for the FromPayload(s) cases where we must not mutate the Payload passed in. I believe it is safe to mutate Payload we receive from our parent DataConverter as those would not yet have been seen by any other code. Let me know if this is not correct.
converter/codec.go
Outdated
| // be nil. The byte slices of the payload's metadata or data should never be | ||
| // mutated directly, but they can be referenced or replaced. | ||
| Encode(*commonpb.Payload) error | ||
| Encode([]*commonpb.Payload) error |
There was a problem hiding this comment.
I think we need to change our strategy a bit and have this return a collection instead of just mutating the parameter so you can encode multiple into one (and same for decode). We can then just change the docs to say "The parameters should never be mutated".
There was a problem hiding this comment.
I'm a bit nervous about changing the payload count, but I agree that returning a slice for these APIs would make them easier to use safely.
There was a problem hiding this comment.
We don't need to change the payload count for our use. We might even should document that we require "at least one" payload response everytime.
|
|
||
| func (z *zlibCodec) Encode(payloads []*commonpb.Payload) ([]*commonpb.Payload, error) { | ||
| result := make([]*commonpb.Payload, len(payloads)) | ||
| for i, p := range payloads { |
There was a problem hiding this comment.
We can now leverage the fact that this operates on multiple payloads and compress them all together.
There was a problem hiding this comment.
That's not something I'm comfortable doing in this PR.
There was a problem hiding this comment.
That is a bit scary, but it could be done, sure. But I'm ok with not doing it here. Really this is an example and I'd be fine removing this codec altogether, heh.
It's actually not the best algorithm, just the best in the stdlib. https://github.com/temporalio/samples-go/tree/main/snappycompress is better.
What was changed
Renamed PayloadEncoder to PayloadCodec. Adjust the API to deal with slices of Payload rather than individual Payload or using a Payloads.
Why?
This is to be more consistent with the naming and interfaces used in other SDKs.