@puellanivis thanks for the review! I'd love to run the CI on it to see if there's anything I've missed.

Hey @dnwe! Of course, I'm happy to make wider changes 🙂

Yours seems like a really nice idea. It should be easy to then add the method to every other request, at which point we'd essentially have this "version negotiation" for everything I guess.

I'll come up with the changes ASAP!

@trapped 😅 well it was just an initial idea, so don't discard what you have here, but if you have time and motivation then feel free to give it a go on a separate branch and see how it might look

The useful property is that it can all be done as "internal" api changes so we don't have to worry about anyone that might be using the protocol types externally, which gives us more freedom to experiment and redesign as we go along

@dnwe yeah, makes sense 🙂 I'll share a link to another draft PR here soon.

@dnwe for now I've opened a draft and copypasted your snipped practically verbatim to all request/response types: #3170

@trapped thanks! One thing I forgot to take account of in my original snippet was that maxVersion supported by the remote broker can and will exceed the max version of the protocol that we've actually implemented the encoding/decoding for within Sarama. I'd also incorrectly used max where we probably wanted min semantics, as we probably want the old Version field from sarama.Config to pin the maximum versions that are used, and people could set Version to sarama.MaxVersion to opt-in to relying on api versions negotiated usage instead.

e.g., for FetchRequest we've only currently implemented up to V11 but Kafka 2.7 and newer will advertise V12+ – so we need to include that in the sum.

So for MetadataRequest probably something like this:

func (m *MetadataRequest) restrictApiVersion(minVersion, maxVersion int16) error {
	maxEncodedVersion := min(10, maxVersion) // see existing isValidVersion() bound check for max supported by Sarama
	if m.Version < minVersion {
		return fmt.Errorf("%w: unsupported API version %d for %T, supported versions are %d-%d",
			ErrUnsupportedVersion, m.Version, m, minVersion, maxEncodedVersion)
	}
	m.Version = min(m.Version, maxEncodedVersion)
	return nil
}

Hey @dnwe, sorry for the delay 🙂 I've just pushed a commit to #3170 applying the change you requested to both Request and Response types - I'm not super familiar with the Sarama codebase/logic yet so thanks for clarifying what behavior it should implement!