Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Breaking change] Fix calling Records::encode / Records::decode with None #100

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

rukai
Copy link
Collaborator

@rukai rukai commented Nov 25, 2024

Calling encode or decode with the correct compression automatically chosen is very difficult. Providing None results in an error like this:

 1  error[E0283]: type annotations needed
    --> kafka-fetch-rewrite/src/lib.rs:87:88
     |
 87  | ...                   RecordBatchDecoder::decode(&mut records_bytes.clone(), None)
     |                       --------------------------                             ^^^^ cannot infer type of the type parameter `T` declared on the enum `Option`
     |                       |
     |                       required by a bound introduced by this call
     |
     = note: multiple `impl`s satisfying `for<'a> _: Fn(&'a mut bytes::Bytes, kafka_protocol::records::Compression)` found in the following crates: `alloc`, `core`:
             - impl<A, F> std::ops::Fn<A> for &F
               where A: std::marker::Tuple, F: std::ops::Fn<A>, F: ?Sized;
             - impl<Args, F, A> std::ops::Fn<Args> for std::boxed::Box<F, A>
               where Args: std::marker::Tuple, F: std::ops::Fn<Args>, A: std::alloc::Allocator, F: ?Sized;
 note: required by a bound in `kafka_protocol::records::RecordBatchDecoder::decode`
    --> /home/rukai/.cargo/registry/src/index.crates.io-6f17d22bba15001f/kafka-protocol-0.13.0/src/records.rs:495:12
     |
 493 |     pub fn decode<B: ByteBuf, F>(buf: &mut B, decompressor: Option<F>) -> Result<Vec<Record>>
     |            ------ required by a bound in this associated function
 494 |     where
 495 |         F: Fn(&mut bytes::Bytes, Compression) -> Result<B>,
     |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `RecordBatchDecoder::decode`
 help: consider specifying the generic argument
     |
 87  |                                 RecordBatchDecoder::decode(&mut records_bytes.clone(), None::<T>)
     |                                                                                            +++++

For which the fix is to change the None to None::<fn(&mut bytes::Bytes, Compression) -> Result<Bytes>>.
This is a pretty poor experience.

To fix this I am making decode/encode provide the correct None value, and have the original implementation moved to decode_with_custom_compression/encode_with_custom_compression.

@pdeva heads up that you will want to adjust your project to call decode_with_custom_compression instead of decode.

@rukai rukai force-pushed the fix_record_encode branch from aeedf6d to 2f72eaf Compare November 25, 2024 03:14
@davide-baldo
Copy link
Contributor

davide-baldo commented Nov 29, 2024

It's not just a poor experience, without anyhow you can't even match the signature.

I was wrong, you can actually use _ :)

@rukai rukai force-pushed the fix_record_encode branch from 2f72eaf to a689cd2 Compare December 1, 2024 22:17
@rukai
Copy link
Collaborator Author

rukai commented Dec 1, 2024

@tychedelia This PR is a very clear improvement to usability so I'll merge it tomorrow unless I hear back otherwise.

@tychedelia tychedelia added this to the 0.14.0 milestone Dec 2, 2024
@tychedelia tychedelia added the enhancement New feature or request label Dec 2, 2024
Copy link
Owner

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rukai. That's pretty gnarly and should have been caught in review. I still think expanding our support for compression is a good idea and so much prefer this wrapping solution.

@@ -196,9 +196,11 @@ fn fetch_records(
);

let mut fetched_records = partition_response.records.clone().unwrap();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have a test covering the new wrapped version but not a big deal.

@tychedelia tychedelia merged commit 565b89a into tychedelia:main Dec 2, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants