Skip to content

KAFKA-6739: Ignore the presence of headers when down-converting from V2 to V1/V0#4813

Merged
hachikuji merged 5 commits intoapache:trunkfrom
dhruvilshah3:KAFKA-6739
Apr 3, 2018
Merged

KAFKA-6739: Ignore the presence of headers when down-converting from V2 to V1/V0#4813
hachikuji merged 5 commits intoapache:trunkfrom
dhruvilshah3:KAFKA-6739

Conversation

@dhruvilshah3
Copy link
Copy Markdown
Contributor

@dhruvilshah3 dhruvilshah3 commented Apr 3, 2018

Because V1/V0 message formats do not expect a header, ignore their presence when down-converting V2 messages that contain headers.
Added a test-case to verify down-conversion sanity in presence of headers.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@dhruvilshah3
KAFKA-6739: When down-converting from V2 to V0/V1, broker must ignore…
31e9c14 
… any header present in the record.

Added a test case to verify sanity when down-converting records containing headers.
@dhruvilshah3
Add comment.
9c95eed
@dhruvilshah3
Make checkStyle happy :-)
d44531d
hachikuji
hachikuji approved these changes Apr 3, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix!

ijuma
ijuma reviewed Apr 3, 2018
View reviewed changes
Comment thread clients/src/main/java/org/apache/kafka/common/record/AbstractRecords.java Outdated
for (Record record : recordBatchAndRecords.records)
builder.append(record);
for (Record record : recordBatchAndRecords.records) {
// Down-convert this record. Ignore the presence of headers when down-converting to V0 and V1 (see KAFKA-6739).
Copy link
Copy Markdown
Member

@ijuma ijuma Apr 3, 2018

Choose a reason for hiding this comment

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

This is a bit subjective, but I personally prefer to avoid references to tickets unless they have information that is hard to replicate. In this case, the main point is that append will reject a record with headers if the message format doesn't support them, but that it the desired behaviour in this case. So we can just explain it here and the test you added makes sure we don't regress.

Copy link
Copy Markdown
Member

@ijuma ijuma Apr 3, 2018

Choose a reason for hiding this comment

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

The reason for this is that you get the context without having to load a separate page and it's less likely to become stale as the code moves around (comments get stale, but references to external resources are even worse.)

Copy link
Copy Markdown
Contributor

@hachikuji hachikuji Apr 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I tend to agree. The JIRA doesn't add extra information that the comment itself couldn't provide. I think it's clear enough if we simply mention that we do not include headers since older message format versions don't support them.

@hachikuji hachikuji merged commit 719a21f into apache:trunk Apr 3, 2018
hachikuji pushed a commit that referenced this pull request Apr 3, 2018
@dhruvilshah3 @hachikuji
KAFKA-6739; Ignore headers when down-converting from V2 to V0/V1 (#4813)
3cc6a67 
Ignore headers when down-converting to V0/V1 since they are not supported. Added a test-case to verify down-conversion sanity in presence of headers.

Reviewers: Ismael Juma <ismael@juma.me.uk>, Jason Gustafson <jason@confluent.io>
hachikuji pushed a commit that referenced this pull request Apr 3, 2018
@dhruvilshah3 @hachikuji
KAFKA-6739; Ignore headers when down-converting from V2 to V0/V1 (#4813)
3738ffe 
Ignore headers when down-converting to V0/V1 since they are not supported. Added a test-case to verify down-conversion sanity in presence of headers.

Reviewers: Ismael Juma <ismael@juma.me.uk>, Jason Gustafson <jason@confluent.io>
@dhruvilshah3 dhruvilshah3 deleted the KAFKA-6739 branch April 3, 2018 17:57
hachikuji pushed a commit to confluentinc/kafka that referenced this pull request Apr 4, 2018
@dhruvilshah3 @hachikuji
KAFKA-6739; Ignore headers when down-converting from V2 to V0/V1 (apa…
8ae1469 
…che#4813)

Ignore headers when down-converting to V0/V1 since they are not supported. Added a test-case to verify down-conversion sanity in presence of headers.

Reviewers: Ismael Juma <ismael@juma.me.uk>, Jason Gustafson <jason@confluent.io>
ijuma added a commit to confluentinc/kafka that referenced this pull request Apr 25, 2018
@ijuma
Merge remote-tracking branch 'confluent/1.1' into 4.1.x-with-interceptor
98c0a61 
* confluent/1.1: (60 commits)
  MINOR: Fix kafka-run-class for Java 10 (apache#4895)
  KAFKA-6772: Load credentials from ZK before accepting connections (apache#4867)
  KAFKA-6742: TopologyTestDriver error when dealing with stores from GlobalKTable
  MINOR: Mention that -1 disables retention by time (apache#4881)
  KAFKA-6790: Fix Streams processor node broken link (apache#4874)
  MINOR: Java 10 fixes so that the build passes (apache#4839)
  MINOR: Update Jackson to 2.9.5 (apache#4776)
  MINOR: Downgrade to Gradle 4.5.1 (apache#4791)
  MINOR: Java 9/10 fixes, gradle and minor deps update (apache#4725)
  KAFKA-6752: Enable unclean leader election metric (apache#4838)
  KAFKA-6054: Fix upgrade path from Kafka Streams v0.10.0 (apache#4773)
  KAFKA-6747 Check whether there is in-flight transaction before aborting transaction (apache#4826)
  KAFKA-6748: double check before scheduling a new task after the punctuate call (apache#4827)
  KAFKA-6739; Ignore headers when down-converting from V2 to V0/V1 (apache#4813)
  KAFKA-6728: Corrected the worker’s instantiation of the HeaderConverter
  KAFKA-6731: waitOnState should check the state to be the target start. (apache#4808)
  HOTFIX: Enforce a rebalance upon task migration (apache#4802)
  MINOR: Remove 1.2.0 changes from streams doc (apache#4784)
  MINOR: Update version numbers to 1.1.1-SNAPSHOT
  MINOR: Fix ReassignPartitionsClusterTest.testHwAfterPartitionReassignment test (apache#4781)
  ...
akatona84 pushed a commit to akatona84/kafka that referenced this pull request Jul 30, 2018
@dhruvilshah3
KAFKA-6739; Ignore headers when down-converting from V2 to V0/V1 (apa…
867242b 
…che#4813)

Ignore headers when down-converting to V0/V1 since they are not supported. Added a test-case to verify down-conversion sanity in presence of headers.

Change-Id: Ib32a22b618bff3bea171f0f052390abb13d6d071
Reviewers: Ismael Juma <ismael@juma.me.uk>, Jason Gustafson <jason@confluent.io>
zmstone added a commit to klarna/kafka that referenced this pull request Nov 6, 2018
@zmstone
Downvert message to v0 or v1 without headers
effe1ba 
This patch is copied from upstram PR: apache#4813
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ijuma ijuma ijuma left review comments

@hachikuji hachikuji hachikuji approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dhruvilshah3 @ijuma @hachikuji