MINOR: Fix handling of dummy record in EndToEndLatency tool by apovzner · Pull Request #5950 · apache/kafka

apovzner · 2018-11-27T05:58:37Z

EndToEndLatency tool produces a dummy record in case the topic does not exist. This behavior was introduced in this PR #5319 as part of updating the tool to use latest consumer API. However, if we run the tool with producer acks == 1, the high watermark may not be updated before we reset consumer offsets to latest. In rare cases when this happens, the tool will throw an exception in the for loop where the consumer will unexpectedly consume the dummy record. As a result, we occasionally see Benchmark.test_end_to_end_latency system test failures.

This PR checks if topic exists, and creates the topic using AdminClient if it does not exist.

Committer Checklist (excluded from commit message)

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

ewencp · 2018-11-27T06:39:51Z


    // sends a dummy message to create the topic if it doesn't exist
-    producer.send(new ProducerRecord[Array[Byte], Array[Byte]](topic, Array[Byte]())).get()
+    val dummyRecord = producer.send(new ProducerRecord[Array[Byte], Array[Byte]](topic, Array[Byte]())).get()


Any reason not to just switch to AdminClient and validate topic creation before proceeding? It seems like this wasn't mentioned in the previous discussion on the PR that introduced this dummy message, but maybe I scanned over it too quickly.

I think we may have done it this way to preserve the functionality in that we wouldn't fail on a cluster with topic auto creation, but arguably the old implementation pre-dummyRecord was simply buggy since at least the first message would incur the topic creation overhead and skew results. Main reason for being careful about changing it is that failing fast would result in an error exit status rather than just skewed results. On the other hand, this code doesn't even work with a topic without auto topic creation on, even if the principal has topic creation permissions, so I'm not sure this is really the right behavior either.

@hachikuji, looks like you reviewed the previous update. Thoughts?

Yeah, using the AdminClient seems preferable over relying on auto topic creation. One disadvantage is that clusters older than 0.10.1 would no longer be supported unless the topic was created manually. But this seems OK since creating the topic manually is not a big deal if someone was trying to compare the performance of very old clusters.

There was no reason not to use AdminClient, except trying to preserve the functionality as closely as possible. However, after reading comments on the previous PR (5319), it seems like the behavior was not really well defined. So, I agree with using AdminClient for topic creation. I changed the code to check if topic exists and use AdminClient to create the topic.

ewencp

I'm basically +1 other than an issue with AdminClient props.

great catch by the way, this seems like it would be a difficult issue to track down!

ewencp · 2018-11-28T04:19:23Z

+    println("Topic \"%s\" does not exist. Will create topic with %d partition(s) and replication factor = %d"
+              .format(topic, defaultNumPartitions, defaultReplicationFactor))
+
+    val props = new Properties()


I think you want to loadProps here like the producer and consumer do. Otherwise this wouldn't work with, e.g. security. Not sure if we're actually leveraging extra props anywhere, but I think we probably want to be consistent.

Ah good point about security and consistency. Will fix.

ewencp · 2018-11-28T04:21:22Z

oh, also @apovzner any idea how far back this should be cherry-picked?

apovzner · 2018-11-29T01:54:53Z

JDK 8 build failure looks unrelated to this change: kafka.admin.ResetConsumerGroupOffsetTest > testResetOffsetsNotExistingGroup: java.util.concurrent.ExecutionException: org.apache.kafka.common.errors.CoordinatorNotAvailableException: The coordinator is not available.

retest this please

apovzner · 2018-11-29T02:04:57Z

@ewencp Thanks for the review. I addressed the AdminClient issue and updated the PR.

About how far back this should be cherry-picked, definitely to 2.1.0. We need this in earlier versions as well, at least back to 1.0. However, earlier versions do not have PR #5319 change, so EndToEndLatency uses different consumer API. I am thinking maybe it is better to open another PR for earlier versions that contain this change and changes to EndToEndLatency in PR #5319?

ijuma · 2018-11-29T02:08:38Z

I suggest just 2.1.0 and trunk.

apovzner · 2018-11-29T22:11:50Z

build failed due to "kafka.server.ListOffsetsRequestTest.testResponseIncludesLeaderEpoch", unrelated.

ewencp · 2018-11-30T06:17:56Z

LGTM! Going to merge to trunk and 2.1. If we want to clear things up for earlier versions as well, we can follow up with another PR.

EndToEndLatency tool produces a dummy record in case the topic does not exist. This behavior was introduced in this PR #5319 as part of updating the tool to use latest consumer API. However, if we run the tool with producer acks == 1, the high watermark may not be updated before we reset consumer offsets to latest. In rare cases when this happens, the tool will throw an exception in the for loop where the consumer will unexpectedly consume the dummy record. As a result, we occasionally see Benchmark.test_end_to_end_latency system test failures. This PR checks if topic exists, and creates the topic using AdminClient if it does not exist. Author: Anna Povzner <anna@confluent.io> Reviewers: Ismael Juma <ismael@juma.me.uk>, Ewen Cheslack-Postava <ewen@confluent.io> Closes #5950 from apovzner/fix-EndToEndLatency (cherry picked from commit 3acebe6) Signed-off-by: Ewen Cheslack-Postava <me@ewencp.org>

EndToEndLatency tool produces a dummy record in case the topic does not exist. This behavior was introduced in this PR apache#5319 as part of updating the tool to use latest consumer API. However, if we run the tool with producer acks == 1, the high watermark may not be updated before we reset consumer offsets to latest. In rare cases when this happens, the tool will throw an exception in the for loop where the consumer will unexpectedly consume the dummy record. As a result, we occasionally see Benchmark.test_end_to_end_latency system test failures. This PR checks if topic exists, and creates the topic using AdminClient if it does not exist. Author: Anna Povzner <anna@confluent.io> Reviewers: Ismael Juma <ismael@juma.me.uk>, Ewen Cheslack-Postava <ewen@confluent.io> Closes apache#5950 from apovzner/fix-EndToEndLatency (cherry picked from commit 3acebe6) Signed-off-by: Ewen Cheslack-Postava <me@ewencp.org>

EndToEndLatency tool produces a dummy record in case the topic does not exist. This behavior was introduced in this PR apache#5319 as part of updating the tool to use latest consumer API. However, if we run the tool with producer acks == 1, the high watermark may not be updated before we reset consumer offsets to latest. In rare cases when this happens, the tool will throw an exception in the for loop where the consumer will unexpectedly consume the dummy record. As a result, we occasionally see Benchmark.test_end_to_end_latency system test failures. This PR checks if topic exists, and creates the topic using AdminClient if it does not exist. Author: Anna Povzner <anna@confluent.io> Reviewers: Ismael Juma <ismael@juma.me.uk>, Ewen Cheslack-Postava <ewen@confluent.io> Closes apache#5950 from apovzner/fix-EndToEndLatency

MINOR: Fix handling of dummy record in EndToEndLatency tool

4ae562e

ewencp reviewed Nov 27, 2018

View reviewed changes

Using AdminClient to create topic instead of relying on auto-creation

5a2f31f

ewencp reviewed Nov 28, 2018

View reviewed changes

Loading props from user to AdminClient config

310e9d0

ewencp closed this in 3acebe6 Nov 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

MINOR: Fix handling of dummy record in EndToEndLatency tool - #5950

MINOR: Fix handling of dummy record in EndToEndLatency tool#5950
apovzner wants to merge 3 commits into
apache:trunkfrom
apovzner:fix-EndToEndLatency

apovzner commented Nov 27, 2018 •

edited

Loading

Uh oh!

ewencp Nov 27, 2018

Uh oh!

ijuma Nov 27, 2018

Uh oh!

apovzner Nov 28, 2018

Uh oh!

ewencp left a comment

Uh oh!

ewencp Nov 28, 2018

Uh oh!

apovzner Nov 28, 2018

Uh oh!

ewencp commented Nov 28, 2018

Uh oh!

apovzner commented Nov 29, 2018

Uh oh!

apovzner commented Nov 29, 2018

Uh oh!

ijuma commented Nov 29, 2018

Uh oh!

apovzner commented Nov 29, 2018

Uh oh!

ewencp commented Nov 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

apovzner commented Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

ewencp Nov 27, 2018

Choose a reason for hiding this comment

Uh oh!

ijuma Nov 27, 2018

Choose a reason for hiding this comment

Uh oh!

apovzner Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

ewencp left a comment

Choose a reason for hiding this comment

Uh oh!

ewencp Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

apovzner Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

ewencp commented Nov 28, 2018

Uh oh!

apovzner commented Nov 29, 2018

Uh oh!

apovzner commented Nov 29, 2018

Uh oh!

ijuma commented Nov 29, 2018

Uh oh!

apovzner commented Nov 29, 2018

Uh oh!

ewencp commented Nov 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

apovzner commented Nov 27, 2018 •

edited

Loading