Skip to content

KAFKA-7819: Improve RoundTripWorker#6187

Merged
cmccabe merged 4 commits intoapache:trunkfrom
stanislavkozlovski:KAFKA-7819-misc-roundtripworker-improvements
Mar 21, 2019
Merged

KAFKA-7819: Improve RoundTripWorker#6187
cmccabe merged 4 commits intoapache:trunkfrom
stanislavkozlovski:KAFKA-7819-misc-roundtripworker-improvements

Conversation

@stanislavkozlovski
Copy link
Copy Markdown
Contributor

This patch changes the Trogdor RoundTripWorker to use a long field for maxMessages and makes the consumer group used unique

This patch changes the Trogdor RoundTripWorker to use a `long` field for maxMessages and makes the consumer group used unique
@stanislavkozlovski
Copy link
Copy Markdown
Contributor Author

JDK 11 passed, JDK 8 failed with one test (lost track of which one it is)
retest this please

@cmccabe
Copy link
Copy Markdown
Contributor

cmccabe commented Mar 4, 2019

Thanks, @stanislavkozlovski . Looks good overall. Suggest using a mutex and condition variable rather than busy-waiting.

long lastLogTimeMs = Time.SYSTEM.milliseconds();
while (true) {
try {
lock.lock();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's definitely overkill to lock the whole function body, right? I guess we only have one thread running this now, but it could be an issue if we want to have multiple threads run it in the future.

Let's just replace the AtomicLong with a regular long, and use the lock to protect changes to the unackedSends variable.

private KafkaConsumer<byte[], byte[]> consumer;

private CountDownLatch unackedSends;
private volatile Long unackedSends;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

volatile is not needed here, since this is only accessed under the lock.

@cmccabe
Copy link
Copy Markdown
Contributor

cmccabe commented Mar 20, 2019

retest this please

@cmccabe
Copy link
Copy Markdown
Contributor

cmccabe commented Mar 20, 2019

LGTM

@stanislavkozlovski
Copy link
Copy Markdown
Contributor Author

JDK 8 passed
JDK 11 failed with two tests apparently (https://builds.apache.org/job/kafka-pr-jdk11-scala2.12/3385/console), yet the logs only show one - kafka.api.ConsumerBounceTest > testRollingBrokerRestartsWithSmallerMaxGroupSizeConfigDisruptsBigGroup FAILED, which is unrelated

I think this is good to merge

@cmccabe
Copy link
Copy Markdown
Contributor

cmccabe commented Mar 21, 2019

Failed test is unrelated.

@cmccabe cmccabe merged commit 6217178 into apache:trunk Mar 21, 2019
jarekr pushed a commit to confluentinc/kafka that referenced this pull request Apr 18, 2019
* apache/trunk: (23 commits)
  KAFKA-7986: Distinguish logging from different ZooKeeperClient instances (apache#6493)
  KAFKA-8102: Add an interval-based Trogdor transaction generator (apache#6444)
  MINOR: Fix misspelling in protocol documentation
  KAFKA-8150: Fix bugs in handling null arrays in generated RPC code (apache#6489)
  KAFKA-8014: Extend Connect integration tests to add and remove workers dynamically (apache#6342)
  MINOR: Remove line for testing repartition topic name (apache#6488)
  MINOR: add MacOS requirement to Streams docs
  MINOR: fix message protocol help text for ElectPreferredLeadersResult (apache#6479)
  MINOR: list-topics should not require topic param
  MINOR: Clean up ThreadCacheTest (apache#6485)
  MINOR: Avoid unnecessary collection copy in MetadataCache (apache#6397)
  KAFKA-8142: Fix NPE for nulls in Headers (apache#6484)
  KAFKA-7243: Add unit integration tests to validate metrics in Kafka Streams (apache#6080)
  MINOR: Add verification step for Streams archetype to Jenkins build (apache#6431)
  KAFKA-7819: Improve RoundTripWorker (apache#6187)
  KAFKA-7989: RequestQuotaTest should wait for quota config change before running tests (apache#6482)
  KAFKA-8098: Fix Flaky Test testConsumerGroups
  KAFKA-6958: Add new NamedOperation interface to enforce consistency in naming operations (apache#6409)
  MINOR: capture result timestamps in Kafka Streams DSL tests (apache#6447)
  MINOR: updated names for deprecated streams constants (apache#6466)
  ...
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
RoundTripWorker to should use a long field for maxMessages rather than an int.  The consumer group used should unique as well.

Reviewers: Colin P. McCabe <cmccabe@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants