KAFKA-9492; Ignore record errors in ProduceResponse for older versions#8030
Conversation
|
@rajinisivaram can you please include a reference to the commit where the behavior regressed? |
ijuma
left a comment
ijuma
left a comment
There was a problem hiding this comment.
LGTM, just a minor comment. It would be good for @guozhangwang and/or @tuvtran to double-check since they worked on the original change (f41a5c2, in particular, was meant to address the compatibility issue).
| } | ||
|
|
||
| @Test | ||
| public void produceResponseRecordErrorsTest() { |
There was a problem hiding this comment.
Maybe this should be in ProduceResponseTest? We started writing this type of test in dedicated classes as this class was getting too large.
There was a problem hiding this comment.
@ijuma Thanks for the review. I have moved this test and the other two produce response tests to ProduceResponseTest.
| Errors error = Errors.forCode(partRespStruct.get(ERROR_CODE)); | ||
| long offset = partRespStruct.getLong(BASE_OFFSET_KEY_NAME); | ||
| long logAppendTime = partRespStruct.getLong(LOG_APPEND_TIME_KEY_NAME); | ||
| long logAppendTime = partRespStruct.hasField(LOG_APPEND_TIME_KEY_NAME) ? |
There was a problem hiding this comment.
Should we convert the type of LOG_APPEND_TIME_KEY_NAME from long to Field.Int64? the benefit of using Field.Int64 is shown below.
- remove the duplicate code of converting LOG_APPEND_TIME_KEY_NAME to Field.Int64
- use more readable method - getOrElse
There was a problem hiding this comment.
@chia7712 Thanks for the review. As @hachikuji mentioned, we can convert to use the generated classes, so will leave that for a separate PR.
guozhangwang
left a comment
guozhangwang
left a comment
There was a problem hiding this comment.
Lgtm. Thanks for the catch.
|
@chia7712 @ijuma @hachikuji @guozhangwang Thanks for the reviews. Test failures are unrelated, merging to trunk, 2.5 and 2.4. |
#8030) Fixes NPE in brokers when processing record errors in produce response for older versions. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Ismael Juma <ismael@juma.me.uk>, Jason Gustafson <jason@confluent.io>, Guozhang Wang <wangguoz@gmail.com>
#8030) Fixes NPE in brokers when processing record errors in produce response for older versions. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Ismael Juma <ismael@juma.me.uk>, Jason Gustafson <jason@confluent.io>, Guozhang Wang <wangguoz@gmail.com>
Conflicts: * build.gradle: moved avro plugin definition below newly added test retry plugin. * apache-github/trunk: MINOR: further InternalTopologyBuilder cleanup (apache#8046) MINOR: Add timer for update limit offsets (apache#8047) HOTFIX: Fix spotsbug failure in Kafka examples (apache#8051) KAFKA-9447: Add new customized EOS model example (apache#8031) KAFKA-8164: Add support for retrying failed (apache#8019) HOTFIX: checkstyle for newly added unit test KAFKA-9261; Client should handle unavailable leader metadata (apache#7770) MINOR: Fix typos introduced in KIP-559 (apache#8042) MINOR: Fixing null handilg in ValueAndTimestampSerializer (apache#7679) KAFKA-9113: Clean up task management and state management (apache#7997) MINOR: fix checkstyle issue in ConsumerConfig.java (apache#8038) KAFKA-9491; Increment high watermark after full log truncation (apache#8037) KAFKA-9477 Document RoundRobinAssignor as an option for partition.assignment.strategy (apache#8007) KAFKA-9074: Correct Connect’s `Values.parseString` to properly parse a time and timestamp literal (apache#7568) KAFKA-9492; Ignore record errors in ProduceResponse for older versions (apache#8030)
apache#8030) Fixes NPE in brokers when processing record errors in produce response for older versions. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Ismael Juma <ismael@juma.me.uk>, Jason Gustafson <jason@confluent.io>, Guozhang Wang <wangguoz@gmail.com>
apache#8030) Fixes NPE in brokers when processing record errors in produce response for older versions. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Ismael Juma <ismael@juma.me.uk>, Jason Gustafson <jason@confluent.io>, Guozhang Wang <wangguoz@gmail.com>
5 participants
f41a5c2 caused a regression in version compatibility. We populate record errors in ProduceResponse regardless of the request version. ProduceResponse.toStruct(version) was attempting to populate
record_errorsif errors were present without checking if the version supported this. This PR adds a version check.Committer Checklist (excluded from commit message)