Parsing only known control batches value #1898
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
|
Thanks for your contribution! Could you please sign CLA to make this PR green? 🙏🏼 |
I did sign the CLA, when I try to do it again it says "user with current Github name already signed the CLA". Is there anything I should do to trigger the CLA check again ? |
Oops, hiccup on our side, my apologies. I'll let Diego review this PR 🙏🏼 |
d1egoaz
reviewed
Mar 10, 2021
control_record.go
Outdated
| @@ -55,6 +45,18 @@ func (cr *ControlRecord) decode(key, value packetDecoder) error { | |||
| // UNKNOWN is used to indicate a control type which the client is not aware of and should be ignored | |||
| cr.Type = ControlRecordUnknown | |||
| } | |||
| // we want to parse value only if we are decoding controll record of known type | |||
d1egoaz
reviewed
Mar 10, 2021
control_record.go
Outdated
| @@ -55,6 +45,18 @@ func (cr *ControlRecord) decode(key, value packetDecoder) error { | |||
| // UNKNOWN is used to indicate a control type which the client is not aware of and should be ignored | |||
| cr.Type = ControlRecordUnknown | |||
| } | |||
| // we want to parse value only if we are decoding controll record of known type | |||
| if cr.Type == ControlRecordAbort || cr.Type == ControlRecordCommit { | |||
There was a problem hiding this comment.
what about:
if cr.Type != ControlRecordUnknown
d1egoaz
approved these changes
Mar 10, 2021
There was a problem hiding this comment.
@mmaslankaprv thanks for the contribution.
It looks like we're missing a/some tests in order to check for this issue.
Would be possible to add some test? I'd expect a test to fail before your change, after your change the test should pass and should be part of our continuous test
mmaslankaprv
force-pushed
the
fixed-parsing-control-record-value
branch
from
19133fa
to
942bd4c
Compare
Kafka documentation defines control batches as a single record batches containing data that shouldn't be passed to applications. The record contained inside the control batch consist of a key with well defined format and a value which format is defined by the control batch type. Control batch key format: ``` ControlRecordKey => version type version => INT16 type => INT16 ``` Fixed control batch parsing logic by first parsing the key to check if control batch is of known type and only then parsing corresponding record value. Signed-off-by: Michal Maslanka <michal@vectorized.io>
Added test validating if we correctly decode Kafka control records. Signed-off-by: Michal Maslanka <michal@vectorized.io>
I've added |
mmaslankaprv
force-pushed
the
fixed-parsing-control-record-value
branch
from
942bd4c
to
e299b55
Compare
|
Thanks @mmaslankaprv |
BenPope
added a commit
to BenPope/redpanda
that referenced
this pull request
Mar 18, 2021
Update Sarama to include: IBM/sarama#1898 "Parsing only known control batches value" `rpk topic consume` now uses fetch v11. Related PR: redpanda-data#890 "k/server: Support serialisation for fetch v11 (RackId)" Signed-off-by: Ben Pope <ben@vectorized.io>
BenPope
added a commit
to BenPope/redpanda
that referenced
this pull request
Mar 18, 2021
* Update Sarama to include: IBM/sarama#1898 * Update mocks `rpk topic consume` now uses fetch v11. Related PR: redpanda-data#890 "k/server: Support serialisation for fetch v11 (RackId)" Signed-off-by: Ben Pope <ben@vectorized.io>
BenPope
added a commit
to BenPope/redpanda
that referenced
this pull request
Mar 18, 2021
Fix `rpk topic consume` * Update Sarama to include: IBM/sarama#1898 * Update mocks `rpk topic consume` now uses fetch v11 implemented in: redpanda-data#890 Signed-off-by: Ben Pope <ben@vectorized.io>
BenPope
added a commit
to BenPope/redpanda
that referenced
this pull request
Mar 18, 2021
Fix `rpk topic consume` * Update Sarama to include: IBM/sarama#1898 * Update mocks `rpk topic consume` now uses fetch v11 implemented in: redpanda-data#890 Signed-off-by: Ben Pope <ben@vectorized.io>
BenPope
added a commit
to BenPope/redpanda
that referenced
this pull request
Mar 18, 2021
Fix `rpk topic consume` * Update Sarama to include: IBM/sarama#1898 * Update mocks `rpk topic consume` now uses fetch v11 implemented in: redpanda-data#890 Signed-off-by: Ben Pope <ben@vectorized.io>
joejulian
pushed a commit
to joejulian/redpanda
that referenced
this pull request
Mar 10, 2023
Fix `rpk topic consume` * Update Sarama to include: IBM/sarama#1898 * Update mocks `rpk topic consume` now uses fetch v11 implemented in: redpanda-data#890 Signed-off-by: Ben Pope <ben@vectorized.io>
joejulian
pushed a commit
to joejulian/redpanda
that referenced
this pull request
Mar 24, 2023
Fix `rpk topic consume` * Update Sarama to include: IBM/sarama#1898 * Update mocks `rpk topic consume` now uses fetch v11 implemented in: redpanda-data#890 Signed-off-by: Ben Pope <ben@vectorized.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Kafka documentation defines control batches as a single record batches
containing data that shouldn't be passed to applications. The record
contained inside the control batch consist of a key with well defined
format and a value which format is defined by the control batch type.
Control batch key format:
Fixed control batch parsing logic by first parsing the key to check if
control batch is of known type and only then parsing corresponding
record value.
Signed-off-by: Michal Maslanka michal@vectorized.io