Skip to content

KAFKA-8118; Ensure ZooKeeper clients are closed in tests, fix verification#6456

Merged
rajinisivaram merged 1 commit intoapache:trunkfrom
rajinisivaram:KAFKA-8118-zkclient
Mar 18, 2019
Merged

KAFKA-8118; Ensure ZooKeeper clients are closed in tests, fix verification#6456
rajinisivaram merged 1 commit intoapache:trunkfrom
rajinisivaram:KAFKA-8118-zkclient

Conversation

@rajinisivaram
Copy link
Copy Markdown
Contributor

We verify that ZK clients are closed in tests since these can affect subsequent tests and that makes it hard to debug test failures. But because of changes to ZooKeeper client, we are now checking the wrong thread name. The thread name used now is <creatorThreadName>-EventThread where creatorThreadName varies depending on the test. Fixed verification ZooKeeperTestHarness to check this format and fixed tests which were leaving ZK clients behind. Also added a test to make sure we can detect changes to the thread name when we update ZK clients in future.

Committer Checklist (excluded from commit message)

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

@rajinisivaram rajinisivaram requested a review from omkreddy March 17, 2019 16:08
Copy link
Copy Markdown
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

Nice catch. LGTM if the tests pass.

Copy link
Copy Markdown
Contributor

@omkreddy omkreddy left a comment

Choose a reason for hiding this comment

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

LGTM

@rajinisivaram
Copy link
Copy Markdown
Contributor Author

@ijuma @omkreddy Thanks for the reviews, merging to master and 2.2.

@rajinisivaram rajinisivaram merged commit 6279b03 into apache:trunk Mar 18, 2019
rajinisivaram added a commit that referenced this pull request Mar 18, 2019
…6456)

We verify that ZK clients are closed in tests since these can affect subsequent tests and that makes it hard to debug test failures. But because of changes to ZooKeeper client, we were checking the wrong thread name. The thread name used now is <creatorThreadName>-EventThread where creatorThreadName varies depending on the test. Fixed ZooKeeperTestHarness to check this format and fixed tests which were leaving ZK clients behind. Also added a test to make sure we can detect changes to the thread name when we update ZK clients in future.

Reviewers: Ismael Juma <ismael@juma.me.uk>, Manikumar Reddy <manikumar.reddy@gmail.com>
jarekr pushed a commit to confluentinc/kafka that referenced this pull request Apr 18, 2019
* apache/trunk:
  MINOR: Retain public constructors of classes from public API (apache#6455)
  KAFKA-8118; Ensure ZK clients are closed in tests, fix verification (apache#6456)
  KAFKA-7813: JmxTool throws NPE when --object-name is omitted
  KAFKA-8114: Wait for SCRAM credential propagation in DelegationTokenEndToEndAuthorizationTest (apache#6452)
  KAFKA-8111; Set min and max versions for Metadata requests (apache#6451)
  KAFKA-7855: Kafka Streams Maven Archetype quickstart fails to compile out of the box (apache#6194)
  MINOR: Update code to not use deprecated methods (apache#6434)
  MINOR: Update Trogdor ConnectionStressWorker status at the end of execution (apache#6445)
  KAFKA-8091; Use commitSync to check connection failure in listener update test (apache#6450)
  KAFKA-7027: Add an overload build method in scala (apache#6373)
  MINOR: Fix typos in LogValidator (apache#6449)
  KAFKA-7502: Cleanup KTable materialization logic in a single place (apache#6174)
  KAFKA-7730; Limit number of active connections per listener in brokers (KIP-402)
  KAFKA-8091; Remove unsafe produce from dynamic listener update test (apache#6443)
  MINOR: Fix JavaDocs warnings (apache#6435)
  MINOR: Better messaging for invalid fetch response (apache#6427)
  MINOR: Use Java 8 lambdas in KStreamImplTest (apache#6430)
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
…pache#6456)

We verify that ZK clients are closed in tests since these can affect subsequent tests and that makes it hard to debug test failures. But because of changes to ZooKeeper client, we were checking the wrong thread name. The thread name used now is <creatorThreadName>-EventThread where creatorThreadName varies depending on the test. Fixed ZooKeeperTestHarness to check this format and fixed tests which were leaving ZK clients behind. Also added a test to make sure we can detect changes to the thread name when we update ZK clients in future.

Reviewers: Ismael Juma <ismael@juma.me.uk>, Manikumar Reddy <manikumar.reddy@gmail.com>
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.

3 participants