fix: fill in the Fetch{Request,Response} protocol

[dnwe]

In order to consume zstd-compressed records the consumer needs to send
and receive version 10 FetchRequest/FetchResponses, but they need to do
so in a well-formed manner that adheres to the encoding format.

Ref: https://kafka.apache.org/protocol

Signed-off-by: Dominic Evans [email protected]

[dnwe]

Note: I know it'd be good to add some more testcases around this, I plan to do those in a follow-up PR

[d1egoaz]

I think there is already some testing framework regarding compression algorithms, see 864d473

I have added your commit to my PR so I can run those tests.

On my local tests your patch worked great!

[dnwe]

@d1egoaz yeah I just meant extending the basic encode+decode roundtripping tests across all the available protocol versions and checking the output contains the expected fields as you increase the version

[varun06]

LGTM

[d1egoaz]

regarding the s/MinVersion/MaxVersion change

I'm wonder what would be the implications of that from existing producers/consumers, Would that change any behaviour?

also, I've notices there are more Min/Max versions that would need to be changed too if we keep following this pattern

[dnwe]

@d1egoaz I agree that we should ideally be consistent and I would potentially submit a follow-up PR to make the change across the board — (although my preference would be to just switch to using the ApiVersions call to enable/disable behaviour based on the broker's advertised supported function rather than guessing based on the local config setting)

In terms of change-in-behaviour, I think the new behaviour is slightly better and no worse:

1. the requiredVersion(...) check is only ever used client-side when sending requests to a broker as verification step by comparing against the sarama.Config configured version, nothing is checked on incoming responses
2. the previous behaviour was that if a protocol version wasn't covered by the cases in the requiredVersion switch statement than it would just always permit it regardless and rely on the remote broker rejecting it if it didn't understand it. I slightly changed this logic so that if we fallthrough to the default switch case then we return MaxVersion here which will likely reject it client-side unless the user has configured their sarama Config to the latest Kafka version known by the version of Sarama they are using.

I'd actually be tempted to make the default fallthrough return [4]uint{99, 99, 99, 99} so that it always rejects things client-side if they aren't known in the switch statement

[d1egoaz]

do you want to merge this @dnwe? and I'll rebase #1574 on top of this

[dnwe]

@d1egoaz yeah I'm happy for this to be merged. I'm not a committer myself so I can't actually perform the merge 😀

[d1egoaz]

Huh, I thought you were a committer :D
ping @bai

[d1egoaz]

thanks for the contribution @dnwe