KAFKA-8014: Extend Connect integration tests to add and remove workers dynamically by kkonstantine · Pull Request #6342 · apache/kafka

kkonstantine · 2019-02-28T05:41:47Z

Extend Connect's integration test harness to add the capability to add and remove workers dynamically as well as discover the set of active workers at any point during an integration test.

The current integration tests are extended to test the new capabilities.

Committer Checklist (excluded from commit message)

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

kkonstantine · 2019-02-28T05:43:47Z

@wicknicks @rhauch @hachikuji @ewencp this seems like a good initial extension before we introduce integration tests that test the Connect framework and its protocols.

Flakiness has not been extensively assessed yet. I'll run a set of repeated run on a builder.

rhauch

Looks great, and no major issues. A few very minor questions/comments below.

kkonstantine · 2019-03-01T02:18:53Z

I've added a commit in this PR that should protect against flakiness. Currently an integration test's success depends on the connect cluster's ability to shutdown gracefully. But this should not be the case, since a test should depend on its assertions, but not in a service exiting with status 0. For example, an occasional failure on stopping a KafkaBasedLog might lead to a test failing at times.

By adding a parameter in the builder we are stating our intent to allow for quick shutdowns of the services.

kkonstantine · 2019-03-01T17:41:36Z

Rebased on top of latest trunk, triggering another test run

rhauch

Nice improvement on the exit conditions and checking!

rhauch · 2019-03-01T18:47:23Z

Seems like these might be useful in all integration tests that use this framework. Does it make sense to add them (or something similar) to the EmbeddedConnectCluster class? One, it helps emphasize that EmbeddedConnectCluster is really just for testing purposes and is not intended for embedding in production applications. Second, it allows other integration tests to much more easily do what this test class is doing by waiting until Connect gets to some desirable state.

This thought crossed my mind too. But I think I'd prefer to move incrementally with what we can generalize and bring into EmbeddedConnectCluster. But totally agree. So far I've kept all the assertions in the test classes because I haven't conclusively decided on a pattern.

wicknicks

Thanks, @kkonstantine! Some questions/comments.

kkonstantine · 2019-03-05T00:41:50Z

@rhauch @wicknicks latest build was green :)
Let me know if you have additional comments.
Will rebase a bit frequently to keep this PR up-to-date and use it as a base for KIP-415 PR as well.

rhauch

A few super minor suggestions, and one question that I'd wish I'd seen earlier.

kkonstantine · 2019-03-06T20:57:46Z

Thanks @rhauch
Good catch on stop. I believe I now have addressed all the pending comments.
@wicknicks anything else that catches your attention?

rhauch

Excellent work, @kkonstantine. LGTM.

Thoughts, @wicknicks, since you've reviewed earlier versions of this PR? Also, how far back do we want to merge this?

kkonstantine · 2019-03-08T00:50:40Z

Thanks @rhauch !
I feel the additions can be a useful extension to integration tests.
I see the original PR has been merged/cherry-picked as far as 2.1.1 (please confirm). Of course, you might want to postpone cherry-picking and backporting so it doesn't interfere with any inflight releases (I think 2.2.0 is in RC mode). For KIP-415 that will consume these changes, trunk is fine too

rhauch

Thanks, @kkonstantine. I did notice one more thing about the exit procedures; see below. Everything else looks great.

rhauch · 2019-03-09T01:10:41Z

@wicknicks, any thoughts?

wicknicks

almost there, @kkonstantine. some minor comments, and one comment/question. thanks!

KAFKA-8014: Use class instead of string to get class name to allow discoverability. KAFKA-8014: Choose whether to fail the test on ungraceful service shutdown KAFKA-8014: Prefer getOrDefault when parsing task configs KAFKA-8014: Abstract worker to its own handle KAFKA-8014: Add thread-safety note and log messages KAFKA-8014: Complete masking for exit and halt in embedded services

kkonstantine

Thanks @wicknicks
Your point regarding some missing assertions from the monitorable source was valid. So I added a few more assertions for records and commits in the handles.

@rhauch @wicknicks if you don't mind, since the changes are enough, could you review the latest commit? I hope it's in the right direction.

wicknicks

some minor comments. thanks for the quick turn around on this. really appreciate the changes.

also, were you planning on merging these changes back to 2.0 (since that's where we introduced these integration tests)?

kkonstantine

Thanks @wicknicks. I believe I addressed or replied to all your recent comments.

wicknicks

Thanks, @kkonstantine! LGTM.

kkonstantine · 2019-03-15T15:07:28Z

Thanks @wicknicks !
@rhauch any additional comments on the last commit? Or you think we are good to go?

…s dynamically (#6342) Extend Connect's integration test framework to add or remove workers to EmbeddedConnectCluster, and choosing whether to fail the test on ungraceful service shutdown. Also added more JavaDoc and other minor improvements. Author: Konstantine Karantasis <konstantine@confluent.io> Reviewers: Arjun Satish <arjun@confluent.io>, Randall Hauch <rhauch@gmail.com> Closes #6342 from kkonstantine/KAFKA-8014

* 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) ...

…s dynamically (apache#6342) Extend Connect's integration test framework to add or remove workers to EmbeddedConnectCluster, and choosing whether to fail the test on ungraceful service shutdown. Also added more JavaDoc and other minor improvements. Author: Konstantine Karantasis <konstantine@confluent.io> Reviewers: Arjun Satish <arjun@confluent.io>, Randall Hauch <rhauch@gmail.com> Closes apache#6342 from kkonstantine/KAFKA-8014

kkonstantine force-pushed the kafka-8014 branch from fec691e to 1979434 Compare February 28, 2019 05:51

rhauch reviewed Feb 28, 2019

View reviewed changes

kkonstantine force-pushed the kafka-8014 branch from 1979434 to 9104b6c Compare March 1, 2019 00:00

kkonstantine force-pushed the kafka-8014 branch from cc6ca95 to 4a4359e Compare March 1, 2019 17:41

rhauch reviewed Mar 1, 2019

View reviewed changes

wicknicks reviewed Mar 2, 2019

View reviewed changes

kkonstantine force-pushed the kafka-8014 branch 3 times, most recently from 865f468 to 8916bd9 Compare March 3, 2019 02:58

kkonstantine force-pushed the kafka-8014 branch from ec11f3e to 3278e92 Compare March 5, 2019 02:48

rhauch reviewed Mar 6, 2019

View reviewed changes

kkonstantine force-pushed the kafka-8014 branch 2 times, most recently from 4a7e4e8 to 8827044 Compare March 6, 2019 20:52

kkonstantine mentioned this pull request Mar 6, 2019

KAFKA-5505: Incremental cooperative rebalancing in Connect (KIP-415) #6363

Merged

3 tasks

rhauch approved these changes Mar 8, 2019

View reviewed changes

rhauch reviewed Mar 8, 2019

View reviewed changes

Comment thread ...ect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedConnectCluster.java Outdated

wicknicks approved these changes Mar 11, 2019

View reviewed changes

kkonstantine added 3 commits March 11, 2019 16:11

KAFKA-8014: Retire workerhandle process and use a facade instead

0268555

KAFKA-8014: Improve javadoc

3719414

kkonstantine force-pushed the kafka-8014 branch from 8827044 to 5e9a1e2 Compare March 11, 2019 23:12

kkonstantine commented Mar 11, 2019

View reviewed changes

KAFKA-8014: Extend handles to record production and commits too

5d9e757

kkonstantine force-pushed the kafka-8014 branch from 5e9a1e2 to 5d9e757 Compare March 12, 2019 00:47

wicknicks reviewed Mar 12, 2019

View reviewed changes

KAFKA-8014: Fix minor issues

881a6d7

kkonstantine commented Mar 12, 2019

View reviewed changes

Comment thread ...untime/src/test/java/org/apache/kafka/connect/integration/ExampleConnectIntegrationTest.java Outdated

Comment thread ...untime/src/test/java/org/apache/kafka/connect/integration/ExampleConnectIntegrationTest.java

wicknicks approved these changes Mar 14, 2019

View reviewed changes

rhauch merged commit e4cad35 into apache:trunk Mar 25, 2019

kkonstantine added the connect label Oct 16, 2020

Conversation

kkonstantine commented Feb 28, 2019

Committer Checklist (excluded from commit message)

Uh oh!

kkonstantine commented Feb 28, 2019

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kkonstantine commented Mar 1, 2019

Uh oh!

kkonstantine commented Mar 1, 2019

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rhauch Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

kkonstantine Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

wicknicks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kkonstantine commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kkonstantine commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

kkonstantine commented Mar 8, 2019

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rhauch commented Mar 9, 2019

Uh oh!

wicknicks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kkonstantine commented Mar 5, 2019 •

edited

Loading

kkonstantine commented Mar 6, 2019 •

edited

Loading