CallistoLabsNYC / samsa

Rust-native Kafka/Redpanda protocol and client implementation.
Apache License 2.0
101 stars 4 forks source link

The comment of `take_varint` Is Confusing #65

Open weixcloud opened 5 months ago

weixcloud commented 5 months ago
// NOTE:: Redpanda sends us values that are 2* the correct value... so make sure to
// divide your values by 2 when you go to use them
pub fn take_varint<E>(i: NomBytes) -> nom::IResult<NomBytes, usize, E>
where

I see the comment points out the value has to be divided by 2 before being used. and It is needed ONLY for redpanda. But I see, in other places of the code, the value is always divided by 2 without checking whether the client is connecting to kafka or redpanda. For example:

fn parse_record(s: NomBytes) -> IResult<NomBytes, Record> {
    let (s, length) = parser::take_varint(s)?;
    let (s, attributes) = be_i8(s)?;
    let (s, timestamp_delta) = parser::take_varint(s)?;
    let (s, offset_delta) = parser::take_varint(s)?;
    let (s, key_length) = parser::take_varint(s)?;
    let (s, key) = take(key_length / 2)(s)?;
    let (s, value_len) = parser::take_varint(s)?;
    let (s, value) = take(value_len / 2)(s)?;

So, I am wondering if the code still works for both kafka and redpanda? Thanks

hgm-king commented 5 months ago

Well, all of our deep protocol work has been through Redpanda so that is what we know best at the moment. The comment doesn't necessarily mean that the two systems' behaviors are different, just I was surprised to have to divide and multiply by 2 for the current tests to work.

When we solidify the api and behavior we aim to codify what works with each system. Our tests have been ran against Kafka and did not show any immediate errors so I believe they should not be any disconnect. We will look deeper into this as we mature the project