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

kafka: fix Sarama control record issue #3189

Merged
merged 1 commit into from
Dec 9, 2021

Conversation

rystsov
Copy link
Contributor

@rystsov rystsov commented Dec 8, 2021

Cover letter

Kafka protocol specifies that the value of a control record is dependent on the type and is opaque to clients. However some clients including Sarama depends on Kafka implementation details

See redpanda-data/connect#953

Fixing the compatibility by returning the same structure as Sarama expects.

Since we pass control records to a client as is and don't look into its value the change isn't only backward compatible with previous Redpanda versions but also is a rollback safe.

Fixes: https://github.com/vectorizedio/support/issues/179

This is where Kafka formats the value:

https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/record/EndTransactionMarker.java

Release notes

Makes Redpanda transactions compatible with Sarama.

src/v/cluster/rm_stm.cc Outdated Show resolved Hide resolved
Kafka protocol specifies that the value of a control record is
dependent on the type and is opaque to clients. However some
clients including Sarama depends on Kafka's implementation details

See redpanda-data/connect#953

Fixing the compatibility by returning the same structure as Sarama
expects.

Since we pass control records to a client as is and don't look
into its value the change isn't only backward compatible with
previous Redpanda versions but also is a rollback safe.

Fix: https://github.com/vectorizedio/support/issues/179
Copy link
Contributor

@VadimPlh VadimPlh left a comment

Choose a reason for hiding this comment

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

LGTM

Only worry about testing. But as I understand you(@rystsov) scheduled it

@dotnwat
Copy link
Member

dotnwat commented Dec 8, 2021

Since we pass control records to a client as is and don't look into its value the change isn't only backward compatible with previous Redpanda versions but also is a rollback safe.

@rystsov thanks that was my first question!

by chance, do you have a reference into Kafka source where the value is constructed?

@rystsov
Copy link
Contributor Author

rystsov commented Dec 9, 2021

Since we pass control records to a client as is and don't look into its value the change isn't only backward compatible with previous Redpanda versions but also is a rollback safe.

@rystsov thanks that was my first question!

by chance, do you have a reference into Kafka source where the value is constructed?

no, I don't I used Sarama sources to deduce types (the value should be opaque so it's safe to return any value)

This pull request was closed.
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.

5 participants