Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ApiVersions rewrite unit test #1832

Merged
merged 1 commit into from
Dec 9, 2024
Merged

Conversation

rukai
Copy link
Member

@rukai rukai commented Nov 21, 2024

Background on ApiVersions

The ApiVersions response is used by the broker to describe which message types and which versions of those message types are supported by the broker.
This allows clients to make use of new functionalities where possible but fallback to more basic function if the new message versions are not supported by the broker.

The problem

Consider the following scenario:

  • Broker supports message type Foo V2
  • Client supports message type Foo only up to V1
  • Shotover can parse Foo V1 and Foo V2 via kafka-protocol crate.
    • However, shotover's rewrite or routing logic for Foo is only compatible with Foo v1. Foo V2 has a new field that needs to be taken into account but since the client currently only supports Foo V1 we have no way to observe this incompatibility in an integration test.

A new version of the client is then released that makes use of Foo v2.
A user begins using this new client before shotover has a chance to discover this issue.
Kafka/shotover breaks in strange ways and everyone is sad.

Thankfully kafka gives us the tools to prevent this from happening, via the ApiVersions message type.

Solution

We can rewrite the ApiVersions response to squeeze the version values into something that shotover is compatible with.
This means:

  • we take the max version supported by the broker and lower it to what shotover is compatible with. (But never increase it, we need to remain compatible with the broker)
  • we take the min version supported by the broker and increase it to what shotover is compatible with. (But never decrease it, we need to remain compatible with the broker)

Note that I'm not aware of kafka itself or shotover ever setting a min value other than 0.
But we should be prepared for kafka one day increasing a minimum value and handle it assuming they might.

This solution is already merged in another PR

Another problem

However this introduces another problem.
Its now very easy for shotover to fall behind on supported messages.
Keeping track of the 81 (and growing) message types and all of their versions is a non-trival task.
The consequences of this are shotover missing new features and efficiencies gained from the new message versions, that it would have been able to support with no changes if it werent for the ApiVersions response being rewritten.

Solving "Another problem"

To make this problem more tractable I have introduced a unit test that asserts the diff between the message types supported by shotover and the message types supported by the kafka-protocol crate.
kafka-protocol crate is generated from the JSON message type definitions in the upstream kafka repository.

So assuming that:

  • kafka-protocol is regenerated on recent JSONs every now and then
  • and we keep shotover up to date with the new releases of kafka-protocol

We should receive notifications of new message types and versions in the form of a failing unit test when updating kafka-protocol crate.

This PR implements this solution

PR Status

The unittest relies on some changes landing upstream in kafka-protocol crate.
So I will land the implementation separately and leave this as a draft for now.

See:

kafka-protocol 0.14 is released so this PR can go ahead now

@rukai rukai force-pushed the api_versions_rewrite branch from 2ea4810 to 6218a59 Compare November 21, 2024 21:36
Copy link

codspeed-hq bot commented Nov 21, 2024

CodSpeed Performance Report

Merging #1832 will not alter performance

Comparing rukai:api_versions_rewrite (f48889a) with main (d754774)

Summary

✅ 38 untouched benchmarks

@rukai rukai force-pushed the api_versions_rewrite branch 4 times, most recently from 9e3b50a to 6f0e011 Compare November 22, 2024 02:15
@rukai rukai mentioned this pull request Nov 22, 2024
@rukai rukai force-pushed the api_versions_rewrite branch 2 times, most recently from f4de0cb to 1b71e42 Compare November 26, 2024 01:45
@rukai rukai force-pushed the api_versions_rewrite branch 4 times, most recently from a00acd4 to 54f5470 Compare December 4, 2024 03:59
@rukai rukai force-pushed the api_versions_rewrite branch from 54f5470 to f48889a Compare December 8, 2024 21:43
@rukai rukai marked this pull request as ready for review December 8, 2024 21:43
@rukai rukai changed the title ApiVersions rewrite with unit test ApiVersions rewrite unit test Dec 8, 2024
@rukai rukai merged commit 4eed01e into shotover:main Dec 9, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants