KAFKA-5308: TC should handle UNSUPPORTED_FOR_MESSAGE_FORMAT in WriteTxnMarker response #3152

dguy · 2017-05-26T09:52:39Z

Return UNSUPPORTED_MESSAGE_FORMAT in handleWriteTxnMarkers when a topic is not the correct message format.
Remove any TopicPartitions that have same error from those waiting for markers

…ic is not the correct message format. Remove such topicpartitions from those waiting for markers

dguy · 2017-05-26T10:09:37Z

@hachikuji @guozhangwang

asfbot · 2017-05-26T11:15:39Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4424/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-05-26T11:43:53Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4438/
Test PASSed (JDK 7 and Scala 2.11).

guozhangwang · 2017-05-27T00:22:50Z

This test failure seems new? kafka.server.LogRecoveryTest.testHWCheckpointWithFailuresSingleLogSegment

guozhangwang

Left some minor comments, otherwise LGTM!

guozhangwang · 2017-05-27T00:32:48Z

...src/main/scala/kafka/coordinator/transaction/TransactionMarkerRequestCompletionHandler.scala

+                    case Errors.UNSUPPORTED_FOR_MESSAGE_FORMAT =>
+                      // The producer would have failed to send data to the failed topic so we can safely remove the partition
+                      // from the set waiting for markers
+                      warn(s"Sending $transactionalId's transaction marker from partition $topicPartition has failed with " +


I'd suggest we log it as info instead of warn, since for the case when the topic gets deleted TC just log it as info while giving up sending markers. I think the case of topic get deleted and topic is on lower version are of similar alert-importance.

guozhangwang · 2017-05-27T00:46:00Z

core/src/main/scala/kafka/server/KafkaApis.scala

@@ -1528,6 +1537,11 @@ class KafkaApis(val requestChannel: RequestChannel,
        partition -> MemoryRecords.withEndTransactionMarker(producerId, marker.producerEpoch, endTxnMarker)
      }.toMap

+      if (partitionsWithIncorrectMessageFormat.nonEmpty)
+        errors.put(producerId, partitionsWithIncorrectMessageFormat.map{partition =>


nit: add space before / after {.

guozhangwang · 2017-05-27T00:58:19Z

...scala/unit/kafka/coordinator/transaction/TransactionMarkerRequestCompletionHandlerTest.scala

@@ -187,6 +187,12 @@ class TransactionMarkerRequestCompletionHandlerTest {
    verifyRetriesPartitionOnError(Errors.NOT_ENOUGH_REPLICAS_AFTER_APPEND)
  }

+  @Test


Could we add a unit test for broker-side handling of writeTxnMarker with replica manager mocked to return smaller magic number?

@guozhangwang there are currently 0 unit-tests for this, just like most of KafkaApis. I've been trying to write a unit test for it, but it isn't as simple as just mocking the ReplicaManager

@guozhangwang i managed to add some tests for this

hachikuji · 2017-05-27T03:30:38Z

...src/main/scala/kafka/coordinator/transaction/TransactionMarkerRequestCompletionHandler.scala

@@ -155,6 +155,14 @@ class TransactionMarkerRequestCompletionHandler(brokerId: Int,
                      txnMarkerChannelManager.removeMarkersForTxnId(transactionalId)
                      abortSending = true

+                    case Errors.UNSUPPORTED_FOR_MESSAGE_FORMAT =>


Can we handle UNSUPPORTED_VERSION here as well? Considering your other patch which checks the inter broker protocol version before handling the transactional request, it is possible (if unlikely) that we could hit that error in the middle of a rolling upgrade.

asfbot · 2017-05-30T09:56:19Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4539/
Test PASSed (JDK 7 and Scala 2.11).

asfbot · 2017-05-30T10:03:25Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4524/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-05-30T12:30:04Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4526/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-05-30T12:44:35Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4541/
Test PASSed (JDK 7 and Scala 2.11).

asfbot · 2017-05-30T15:15:31Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4536/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-05-30T16:30:26Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4538/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-05-30T16:45:12Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4553/
Test PASSed (JDK 7 and Scala 2.11).

guozhangwang

One minor comment.

guozhangwang · 2017-05-30T19:57:41Z

clients/src/test/java/org/apache/kafka/common/requests/ByteBufferChannel.java

+import java.nio.ByteBuffer;
+import java.nio.channels.GatheringByteChannel;
+
+public class ByteBufferChannel implements GatheringByteChannel {


Does this need to be in common.requests? Could we put it under common.network?

I'll go ahead and change this when I merge. Assuming @dguy wouldn't mind the different location.

Sounds good. Actually let me make the change-and-merge.

Actually I just realized that this is only extracted as a test util class, and hence it is okay to just leave it as is.

hachikuji

Thanks for the patch. LGTM

…xnMarker response Return UNSUPPORTED_MESSAGE_FORMAT in handleWriteTxnMarkers when a topic is not the correct message format. Remove any TopicPartitions that have same error from those waiting for markers Author: Damian Guy <damian.guy@gmail.com> Reviewers: Jason Gustafson <jason@confluent.io>, Guozhang Wang <wangguoz@gmail.com> Closes #3152 from dguy/kafka-5308 (cherry picked from commit 2cc8f48) Signed-off-by: Guozhang Wang <wangguoz@gmail.com>

guozhangwang

Merged to trunk and 0.11.0.

dguy added 2 commits May 26, 2017 10:49

Return UNSUPPORTED_MESSAGE_FORMAT in handleWriteTxnMarkers when a top…

53ec77f

…ic is not the correct message format. Remove such topicpartitions from those waiting for markers

merge trunk

a68eba7

guozhangwang reviewed May 27, 2017

View reviewed changes

hachikuji reviewed May 27, 2017

View reviewed changes

dguy added 2 commits May 30, 2017 09:00

Merge branch 'trunk' into kafka-5308

eb35084

address comments

c193dec

dguy added 2 commits May 30, 2017 11:31

don't append to log if all errors

da1e57a

add test for handleWriteTxnMarkers

3abb369

couple more tests and fix code

64e3106

fix findbugs

ac03681

guozhangwang reviewed May 30, 2017

View reviewed changes

hachikuji approved these changes May 31, 2017

View reviewed changes

guozhangwang approved these changes May 31, 2017

View reviewed changes

asfgit closed this in 2cc8f48 May 31, 2017

dguy deleted the kafka-5308 branch August 16, 2017 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-5308: TC should handle UNSUPPORTED_FOR_MESSAGE_FORMAT in WriteTxnMarker response #3152

KAFKA-5308: TC should handle UNSUPPORTED_FOR_MESSAGE_FORMAT in WriteTxnMarker response #3152

dguy commented May 26, 2017

dguy commented May 26, 2017

asfbot commented May 26, 2017

asfbot commented May 26, 2017

guozhangwang commented May 27, 2017

guozhangwang left a comment

guozhangwang May 27, 2017

guozhangwang May 27, 2017

guozhangwang May 27, 2017

dguy May 30, 2017

dguy May 30, 2017

hachikuji May 27, 2017 •

edited

Loading

asfbot commented May 30, 2017

asfbot commented May 30, 2017

asfbot commented May 30, 2017

asfbot commented May 30, 2017

asfbot commented May 30, 2017

asfbot commented May 30, 2017

asfbot commented May 30, 2017

guozhangwang left a comment

guozhangwang May 30, 2017

hachikuji May 31, 2017

guozhangwang May 31, 2017

guozhangwang May 31, 2017

hachikuji left a comment

guozhangwang left a comment

KAFKA-5308: TC should handle UNSUPPORTED_FOR_MESSAGE_FORMAT in WriteTxnMarker response #3152

KAFKA-5308: TC should handle UNSUPPORTED_FOR_MESSAGE_FORMAT in WriteTxnMarker response #3152

Conversation

dguy commented May 26, 2017

dguy commented May 26, 2017

asfbot commented May 26, 2017

asfbot commented May 26, 2017

guozhangwang commented May 27, 2017

guozhangwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hachikuji May 27, 2017 • edited Loading

Choose a reason for hiding this comment

asfbot commented May 30, 2017

asfbot commented May 30, 2017

asfbot commented May 30, 2017

asfbot commented May 30, 2017

asfbot commented May 30, 2017

asfbot commented May 30, 2017

asfbot commented May 30, 2017

guozhangwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hachikuji left a comment

Choose a reason for hiding this comment

guozhangwang left a comment

Choose a reason for hiding this comment

hachikuji May 27, 2017 •

edited

Loading