Refactor AMQPCodec #111
Merged
Refactor AMQPCodec #111
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Keruspe
reviewed
May 20, 2018
| start: gen_class(class) >> | ||
| end: gen_at_offset!(len, gen_be_u32!(end - start)) >> | ||
| gen_be_u8!(constants::FRAME_END) | ||
| ) |
Collaborator
There was a problem hiding this comment.
Heh, I was pretty sure I did something similar already... found it: https://github.com/sozu-proxy/amq-protocol/blob/master/protocol/src/frame/generation.rs#L19
I thought I synced that a while ago but apparently I didn't.
Will sync all that stuff soonish after some testing
Collaborator
|
LGTM once we get a new cookie-factory release |
Collaborator
|
Btw, with this we can bring back and use the Sink impl right? |
Contributor
Author
|
Yep, I'm planning on adding a few more tests, I'll also restore the Sink impl |
The `AMQPCodec::encode` function overwrote the contents of the buffer given to it, forcing its users to flush the buffer after encoding each item. The new implementation simplifies `AMQPTransport` and especially allows for the removal of the `flush_needed` attribute.
Collaborator
|
Rebased on current master, now that cookie-factory 0.2.4 has been released |
Contributor
Author
|
Thanks, I'll push the rest tonight or tomorrow |
This will prevent putting too much strain on the allocator in the case of a frame consisting of many parts exceeding the buffer's capacity. Also, the threshold to a reallocation is now 1/4th of a frame, which is 1/8th of the space allocated.
Add a test with a frame more complex than the heartbeat one & a test regarding the behaviour of buffer capacity expansion.
Collaborator
|
Thanks ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
AMQPCodec::encodefunction overwrote the contents of the buffergiven to it, forcing its users to flush the buffer after encoding each
item.
The new implementation simplifies
AMQPTransportand especially allowsfor the removal of the
flush_neededattribute.