feat(protocol): add restrictApiVersion method#3170
Closed
feat(protocol): add restrictApiVersion method#3170
Conversation
Signed-off-by: Giorgio Pellero <[email protected]>
Signed-off-by: Giorgio Pellero <[email protected]>
Signed-off-by: Giorgio Pellero <[email protected]>
Signed-off-by: Giorgio Pellero <[email protected]>
Signed-off-by: Giorgio Pellero <[email protected]>
Signed-off-by: Giorgio Pellero <[email protected]>
Signed-off-by: Giorgio Pellero <[email protected]>
Signed-off-by: Giorgio Pellero <[email protected]>
Signed-off-by: Giorgio Pellero <[email protected]>
Signed-off-by: Giorgio Pellero <[email protected]>
Signed-off-by: Giorgio Pellero <[email protected]>
0b04572 to
848aa8d
Compare
Signed-off-by: Giorgio Pellero <[email protected]>
c20e053 to
6e673e5
Compare
Think this was just missed in the follow-up commit. Pin ProduceRequest to V7 with the same min(...) pattern as the rest. Signed-off-by: Dominic Evans <[email protected]>
Signed-off-by: Dominic Evans <[email protected]>
Signed-off-by: Dominic Evans <[email protected]>
Collaborator
|
Nice, with a few extra commits to account for the updates that were missed, we're impressively getting a fully passing suite :) |
Contributor
Author
|
Oops, my bad - missed those. Thanks for the help! |
Contributor
Author
Collaborator
|
@trapped yeah, now it is in a passing state maybe squash the commits on this branch down into a single one and then have a quick think about whether this is the best structuring of this or how we could perhaps:
|
Contributor
Author
|
Hey @dnwe, sorry for the delay, couldn't find time for this until now. After spending some more time learning the lib's structure I ended up addressing your suggestions and making things a bit neater in the process. To keep things clean I'm closing both the old PR and this one - I've pushed the new changes to #3209 🙂 |
dnwe
pushed a commit
that referenced
this pull request
Jul 11, 2025
This PR introduces basic Kafka API version negotiation in Sarama by enabling the client to restrict the API version for each request based on the version ranges advertised by brokers in the initial `ApiVersions` request/response - which is now executed synchronously as part of the broker connection process, rather than as a fire-and-forget deferred goroutine, and before SASL authentication takes place (but after SSL/TLS, as indicated by the [Kafka protocol docs](https://kafka.apache.org/26/protocol.html#api_versions)). The selected version is the highest supported by both the broker and the user-defined `Config.Version` upper bound. To support this dynamic selection, a new `setVersion(int16)` method has been added to the `protocolBody` interface, exposing the request/response structs' `Version` field to be set externally. Also, it defines constants for message keys, and replaces some (read: those I found) usages of numeric literals to use the constants instead. I experimented with extracting more logic from `protocolBody` implementations, too, such as centralizing `requiredVersion()`, `isValidVersion()`, and the "max protocol version for Kafka version" selection in most Request constructors [as suggested by @dnwe in my previous PR](#3170 (comment)); however, while they can certainly simplify the code, I found these were significantly larger-scale changes (and kinda out of scope for the purpose of this PR), and decided to leave them out to keep this PR short and easy to review. <details> <summary>Motivation behind the change: Sarama incompatibility with Redpanda</summary> The latest `github.com/IBM/sarama` version fails to connect to Redpanda brokers (latest version too): - Redpanda rejects the request with `Unsupported version 10 for metadata API` - sarama fails with `kafka: client has run out of available brokers to talk to: EOF` <details> <summary>Sarama logs</summary> ``` 2025/05/13 18:26:29 Initializing new client 2025/05/13 18:26:29 ClientID is the default of 'sarama', you should consider setting it to something application-specific. 2025/05/13 18:26:29 ClientID is the default of 'sarama', you should consider setting it to something application-specific. 2025/05/13 18:26:29 client/metadata fetching metadata for all topics from broker localhost:9092 2025/05/13 18:26:29 Connected to broker at localhost:9092 (unregistered) 2025/05/13 18:26:29 client/metadata got error from broker -1 while fetching metadata: EOF 2025/05/13 18:26:29 Closed connection to broker localhost:9092 2025/05/13 18:26:29 client/metadata no available broker to send metadata request to 2025/05/13 18:26:29 client/brokers resurrecting 1 dead seed brokers 2025/05/13 18:26:29 Error while sending ApiVersionsRequest to broker localhost:9092: kafka: broker not connected 2025/05/13 18:26:30 client/metadata retrying after 250ms... (2 attempts remaining) 2025/05/13 18:26:30 ClientID is the default of 'sarama', you should consider setting it to something application-specific. 2025/05/13 18:26:30 client/metadata fetching metadata for all topics from broker localhost:9092 2025/05/13 18:26:30 Connected to broker at localhost:9092 (unregistered) 2025/05/13 18:26:30 client/metadata got error from broker -1 while fetching metadata: EOF 2025/05/13 18:26:30 Closed connection to broker localhost:9092 2025/05/13 18:26:30 client/metadata no available broker to send metadata request to 2025/05/13 18:26:30 client/brokers resurrecting 1 dead seed brokers 2025/05/13 18:26:30 client/metadata retrying after 250ms... (1 attempts remaining) 2025/05/13 18:26:30 ClientID is the default of 'sarama', you should consider setting it to something application-specific. 2025/05/13 18:26:30 client/metadata fetching metadata for all topics from broker localhost:9092 2025/05/13 18:26:30 Connected to broker at localhost:9092 (unregistered) 2025/05/13 18:26:30 client/metadata got error from broker -1 while fetching metadata: EOF 2025/05/13 18:26:30 Closed connection to broker localhost:9092 2025/05/13 18:26:30 client/metadata no available broker to send metadata request to 2025/05/13 18:26:30 client/brokers resurrecting 1 dead seed brokers 2025/05/13 18:26:30 client/metadata retrying after 250ms... (0 attempts remaining) 2025/05/13 18:26:30 ClientID is the default of 'sarama', you should consider setting it to something application-specific. 2025/05/13 18:26:30 client/metadata fetching metadata for all topics from broker localhost:9092 2025/05/13 18:26:30 Connected to broker at localhost:9092 (unregistered) 2025/05/13 18:26:30 client/metadata got error from broker -1 while fetching metadata: EOF 2025/05/13 18:26:30 Closed connection to broker localhost:9092 2025/05/13 18:26:30 client/metadata no available broker to send metadata request to 2025/05/13 18:26:30 client/brokers resurrecting 1 dead seed brokers 2025/05/13 18:26:30 Closing Client ``` </details> The culprit being simply that: 1. Sarama chooses request versions solely according to the provided `config.Version`, [even though it sends an ApiVersionsRequest](https://github.com/IBM/sarama/blob/43f8559f9dc49d7bc46f81adec99aed43d423bf8/broker.go#L189-L201) 4. [MetadataRequest uses versions 8-9-10 for Kafka versions >= v2.4.0.0](https://github.com/IBM/sarama/blob/43f8559f9dc49d7bc46f81adec99aed43d423bf8/metadata_request.go#L26-L29) 5. [Redpanda only supports versions 0 to 8](https://github.com/redpanda-data/redpanda/blob/bee4479bf54ae2d9af54b07b1203179791b0f166/src/v/kafka/server/handlers/metadata.h#L28), and it correctly advertises it </details> --------- Signed-off-by: Giorgio Pellero <[email protected]>
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
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.
No description provided.