Add helper methods to RequestKind and ResponseKind #70
Conversation
This file contains 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
tychedelia
approved these changes
Aug 27, 2024
There was a problem hiding this comment.
Makes sense to me!
Per your comment about features and compile times, I think that makes a ton of sense. The enums also tend to be less useful for clients. I'm going to put this in a 0.12 milestone with the assumption we can start at least with those features and maybe a client/broker feature later if you have time. 👍
Thanks for the work!
| module_file, | ||
| r#" | ||
| fn decode<T: Decodable>(bytes: &mut bytes::Bytes, version: i16) -> Result<T> {{ | ||
| T::decode(bytes, version).with_context(|| {{ |
rukai
force-pushed
the
add_methods_to_request_kind_and_response_kind
branch
from
e6bf84a to
3659a77
Compare
|
Thanks for feedback, should be addressed now. |
rukai
force-pushed
the
add_methods_to_request_kind_and_response_kind
branch
from
3659a77 to
4364265
Compare
rukai
force-pushed
the
add_methods_to_request_kind_and_response_kind
branch
from
4364265 to
1406639
Compare
tychedelia
approved these changes
Aug 28, 2024
| @@ -32,4 +32,5 @@ jobs: | |||
| - uses: dtolnay/rust-toolchain@stable | |||
| with: | |||
| components: clippy | |||
| - run: cargo clippy --workspace -- -D warnings | |||
|
@rukai any other changes you have on the horizon or do you want me to release |
|
I'll have a look into the client and broker features first |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR is a first step towards providing a higher level API for encoding and decoding messages.
Given the existence of the RequestKind and ResponseKind enums which has a variant for every possible request/response, it makes sense that the user would be matching on these enums and calling methods on the contained objects.
So this PR generates encode, decode and header_version methods on RequestKind/ResponseKind, to easily call into the contained object.
As an example, it will drastically simplify the implementation of the project I work on, which previously had these matches written out by hand: https://github.com/shotover/shotover-proxy/pull/1736/files
I think that a further high level API can be written on top of this to simplify header/body encoding/decoding.
Also worth noting that its actually impossible to implement such a match on
RequestKind/ResponseKindoutside of the kafka-protocol crate due to the#[non_exhaustive].We currently have to use our own enums instead of kafka-protocol to ensure that we exhaustively handle encoding/decoding every message type.
Compile times
I've measured that this PR increases release build time of kafka-protocol from 7s to 14s.
I dont believe this would impact compile time of the final binary, if these methods are not used.
Still, for cold builds this is pretty bad.
So, wow do you feel about putting
RequestKind/ResponseKindbehind a feature flag so this doesnt impact users who dont touchRequestKind/ResponseKind?I think that
RequestKind/ResponseKind, doesnt really provide any value without these new methods, so I think including them all in a single feature flag sounds reasonable to me.Another possible compile time win
Another improvement to compile time that could help here is to add a
clientfeature and abrokerfeature such that.clientenables encoding for requests and decoding for responsesbrokerenables encoding for responses and decoding for requests.Both features can be enabled for proxy use cases.
This would have the impact for client or broker implementations.
But it would go a in a follow up PR and while it shouldnt be difficult, it will take some time to implement.