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

Add support for flexible messages #562

Merged
merged 18 commits into from
Nov 24, 2020
Merged

Conversation

yolken-segment
Copy link
Contributor

@yolken-segment yolken-segment commented Nov 21, 2020

Description

This change updates kafka-go to support the "flexible" message format described in KIP-482. Among other changes, the new format adds optional "tagged fields" into API messages and also makes the encodings of strings and arrays more compact.

The values of the tagged fields aren't used for much yet (I think), but we need to support the new, "flexible" format to get the latest API versions described in the Kafka protocol guide.

The proposed interface is to support tagged fields via a new, tag= property in the existing protocol struct tags, e.g.:

type Request struct {
    [required fields]

    TaggedField0 `kafka:"min=v1,max=v3,tag=0"`
    TaggedField1 `kafka:"min=v2,max=v3,tag=1"`
   ...
}

As an example and to provide something to test with, this change updates the max version of the CreateTopics API to v5, which uses the new format.

Misc. notes and TODOs

  1. I've added Kafka 2.4.1 into the CI since something >= 2.4 is needed to exercise the new message formats. However, the nettest suite fails for this version, so I'm temporarily adding an option to skip this. I get the same failures when running from the base 0.4 branch, so I don't think these errors are related to the changes here.
  2. KIP-482 defines new, "compact" encodings that are different from the ones currently defined in the kafka-go procotol. I've renamed the latter from "compact" to "var", e.g. VarString, VarBytes, etc. to distinguish them.
  3. Non-empty tag buffers aren't included yet in any real APIs (as far as I can tell), so it's hard to be sure that the implementation here is 100% aligned with what Kafka will expect in the future. We may need to make fixes in the future once tagged fields are actively set.

protocol/createtopics/createtopics.go Outdated Show resolved Hide resolved
protocol/createtopics/createtopics.go Outdated Show resolved Hide resolved
protocol/size.go Outdated Show resolved Hide resolved
working_directory: *working_directory
environment:
KAFKA_VERSION: "2.4.1"
KAFKA_SKIP_NETTEST: "1"
Copy link
Contributor

@achille-roussel achille-roussel Nov 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind explaining why these tests cannot pass with 2.4?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Also added a similar comment to where KAFKA_SKIP_NETTEST is read in the code.

Copy link
Contributor

@achille-roussel achille-roussel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great 🙌

@yolken-segment yolken-segment merged commit 7baf347 into 0.4 Nov 24, 2020
@yolken-segment yolken-segment deleted the yolken-04-flexible-messages branch November 24, 2020 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants