Add Read and WriterTo methods to envelopes #622
Conversation
Allow src and dst to be set for envelope read/writers. Envelopes also implement io.Reader and io.WriterTo methods to allow passing envelopes as a message payload.
| if !env.IsSet(flagEnvelopeCompressed) && | ||
| w.compressionPool != nil && | ||
| env.Data.Len() > w.compressMinBytes { | ||
| if err := w.compress(env.Data); err != nil { |
There was a problem hiding this comment.
Behaviour change on Write, now mutates the *env to compress, if needed. Nothing currently depended on the *env being unchanged.
There was a problem hiding this comment.
These kinds of changes (mostly code motion) are sooo much easier to review if you don't change behavior at all. Was this change actually necessary? If not, please just omit and maybe do in a follow-up.
| e.offset += n | ||
| wroteN += int64(n) | ||
| return wroteN, err | ||
| } |
There was a problem hiding this comment.
For #611 will also need to add func (e *envelope) Rewind() { b.offset = 0}
There was a problem hiding this comment.
I think it would be way cleaner if envelope were immutable once sent. So that means we'd instead want a NewReader() io.Reader method, so that thing can be mutable/rewindable. That way, if any synchronization becomes necessary (as exists in the payload thingie in #611), it can live in that reader impl and not leak into the envelope struct itself.
jhump
changed the title
There was a problem hiding this comment.
I don't see why we're changing from currying the writer in the envelopeWriter to not. That seems unrelated and unnecessary.
I know you stated in the description that you don't see why it's "necessary", but I'm afraid I don't see why it's necessary to change. This seems like aesthetic choice (IMO, low value) that expands the blast radius of this PR by several files and dozens of call sites. If it's not needed for the change, let's please leave it out. In fact, it seems like you'll end up having to undo/redo it anyway to actually change duplexHTTPCall so it has a send(*envelope) method instead of implementing io.Writer.
In fact, I think it's actually worth making that change (changing duplexHTTPCall to actually use what's in this branch) in this PR, so we can see what that looks like even before the unary-request specialization change.
| if !env.IsSet(flagEnvelopeCompressed) && | ||
| w.compressionPool != nil && | ||
| env.Data.Len() > w.compressMinBytes { | ||
| if err := w.compress(env.Data); err != nil { |
There was a problem hiding this comment.
These kinds of changes (mostly code motion) are sooo much easier to review if you don't change behavior at all. Was this change actually necessary? If not, please just omit and maybe do in a follow-up.
| e.offset += n | ||
| wroteN += int64(n) | ||
| return wroteN, err | ||
| } |
There was a problem hiding this comment.
I think it would be way cleaner if envelope were immutable once sent. So that means we'd instead want a NewReader() io.Reader method, so that thing can be mutable/rewindable. That way, if any synchronization becomes necessary (as exists in the payload thingie in #611), it can live in that reader impl and not leak into the envelope struct itself.
I don't understand what you are asking for. How can I call a different method on
I didn't, but they aren't. Why do we have a different call sites for unary vs envelope? The only difference should be the prefix which we can cover by wrapping in an envelope payload. I'll make a proposal for the work and post it in the issue. |
You can't, hence me suggesting that the changes here will need to be undone/re-done to weave a different type through.
Sorry, I guess I need to go look at a little more of the code to get adequate context for this. |
|
Closing, will discuss the internal API changes needed in #609 |
Allow
srcanddstto be set for envelope read/writers. Envelopes also implementio.Readerandio.WriterTomethods to allow passing envelopes as a message payload.Requirement for #611 to avoid copying the buffers. See #609