sarama icon indicating copy to clipboard operation
sarama copied to clipboard

feat(protocol): add restrictApiVersion method

Open trapped opened this issue 7 months ago • 4 comments

trapped avatar May 20 '25 07:05 trapped

Nice, with a few extra commits to account for the updates that were missed, we're impressively getting a fully passing suite :)

dnwe avatar May 28 '25 09:05 dnwe

Oops, my bad - missed those. Thanks for the help!

trapped avatar May 28 '25 09:05 trapped

@dnwe shall I close https://github.com/IBM/sarama/pull/3158 and move forward with this one instead? If so I'll add a few tests to cover the changes

trapped avatar Jun 07 '25 14:06 trapped

@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:

  1. decide if requiredVersion is still necessary once restrictApiVersion exists
  2. consider storing the "max-implemented" version numbers for each protocol api key in a single location and use those in both requiredVersion (if still required) and restrictApiVersion?
  3. consider whether the logic of restrictApiVersion itself could be centralised? Originally we added it as a func on each protocol type because that's the easiest way to allow it to modify the Version field on the struct, but perhaps the func on each protocol could just be a shim to a generic shared func?

dnwe avatar Jun 09 '25 09:06 dnwe

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 https://github.com/IBM/sarama/pull/3209 🙂

trapped avatar Jun 27 '25 18:06 trapped