KAFKA-9273: Extract testShouldAutoShutdownOnIncompleteMetadata from S… by albert02lowis · Pull Request #9108 · apache/kafka

albert02lowis · 2020-07-31T04:35:15Z

…treamTableJoinIntegrationTest into its own test

The main goal is to remove usage of embedded broker (EmbeddedKafkaCluster) in AbstractJoinIntegrationTest and its subclasses.
This is because the tests under this class are no longer using the embedded broker, except for two.
testShouldAutoShutdownOnIncompleteMetadata is one of such tests.
Furthermore, this test does not actually perfom stream-table join; it is testing an edge case of joining with a non-existent topic, so it should be in a separate test.

Testing strategy: run existing unit and integration test

@bbejeck

Committer Checklist (excluded from commit message)

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

abbccdda

@bbejeck Could you take a look to make sure this is the only test which needs to be moving out?

abbccdda · 2020-07-31T17:24:43Z

retest this please

…treamTableJoinIntegrationTest into its own test The main goal is to remove usage of embedded broker (EmbeddedKafkaCluster) in AbstractJoinIntegrationTest and its subclasses. This is because the tests under this class are no longer using the embedded broker, except for two. testShouldAutoShutdownOnIncompleteMetadata is one of such tests. Furthermore, this test does not actually perfom stream-table join; it is testing an edge case of joining with a non-existent topic, so it should be in a separate test.

albert02lowis · 2020-08-01T03:44:37Z

Hi, there's another unit test that need to be moved out (StreamStreamJoinIntegrationTest.shouldNotAccessJoinStoresWhenGivingName) but I thought of doing that in another PR (to make the PRs small). But let me know if it's ok to just include it in this PR.

(Btw, I force-push updated the commit with the right user email)

abbccdda · 2020-08-01T04:26:30Z

@albert02lowis I see, let's try to do all of them in one PR then.

…amStreamJoinIntegrationTest into its own test. shouldNotAccessJoinStoresWhenGivingName is the last unit test that has a dependency to embedded broker in AbstractJoinIntegrationTest. This test does not actually test the behaviour of stream-stream join; It is testing an edge case of accessing the store of a join, so it should be in a separate test.

…egrationTest. The tests under AbstractJoinIntegrationTest previously requires embedded broker, but it is better to use TopologyTestDriver instead. Existing stream-stream join tests that still make use of embedded broker is moved to use TopologyTestDriver, by specifying it under BOOTSTRAP_SERVERS_CONFIG. And then the old EmbeddedKafkaCluster in AbstractJoinIntegrationTest is safely removed.

albert02lowis · 2020-08-03T07:14:32Z

Hi @abbccdda sure thing, I have added 2 more commits to this PR and the unit + integration tests also looks good in my local

bbejeck · 2020-08-12T13:53:53Z

Ok to test

bbejeck

@albert02lowis thanks for the contribution. Overall the PR looks good to me, after the one comment is addressed we'll probably get this merged.

bbejeck · 2020-08-12T22:59:35Z

    public static void setupConfigsAndUtils() {

        STREAMS_CONFIG.put(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, "earliest");
-        STREAMS_CONFIG.put(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG, CLUSTER.bootstrapServers());


@albert02lowis I've confirmed locally that the test failures are related. The TopologyTestDriver still needs a bootstrap servers config value.
Adding something like STREAMS_CONFIG.put(ConsumerConfig.BOOTSTRAP_SERVERS_CONFIG, "test:0000"); into the AbstractJoinIntegrationTest.setupConfigsAndUtils() gets all tests passing.

I'm not sure how the JDK 14 tests passed as locally I got the same errors as the JDK 8 and 11 build locally.

I see, let me add back this BOOTSTRAP_SERVERS_CONFIG inside setupConfigsAndUtils then 👍🏻

Hi @bbejeck I have done the required change in a new commit. I have also extracted out redundant calls to set BOOTSTRAP_SERVERS_CONFIG in the subclasses' test methods.

…egrationTest. Previously the setting of BOOTSTRAP_SERVERS_CONFIG is added on each test method, but this can actually be extracted to the BeforeClass method which is setupConfigsAndUtils in AbstractJoinIntegrationTest.

bbejeck · 2020-08-13T14:18:34Z

retest this please

bbejeck · 2020-08-13T14:20:10Z

Retest this please.

bbejeck · 2020-08-13T14:20:42Z

Ok to test

abbccdda · 2020-08-13T15:39:51Z

retest this please

bbejeck · 2020-08-13T16:28:05Z

ok to test

bbejeck

Thanks for the update @albert02lowis, this LGTM. The streams build passed for me locally. Once we can get a build running here, I'll merge it.

bbejeck · 2020-08-14T14:05:55Z

retest this please

bbejeck · 2020-08-14T16:43:57Z

Java 8 failed on an unrelated test kafka.api.SaslSslConsumerTest.testCoordinatorFailover created a Jira ticket.

retest this please

bbejeck · 2020-08-14T19:06:08Z

Java 8 passed
Java 14 failed with org.apache.kafka.streams.integration.PurgeRepartitionTopicIntegrationTest.shouldRestoreState
Java 11 failed with kafka.api.PlaintextAdminIntegrationTest.testAlterReplicaLogDirs

retest this please

bbejeck · 2020-08-15T12:58:47Z

Java 11 passed
Java 14 failed due to Execution failed for task ':connect:api:javadoc'. ? (0 tests failed
Java 8 appears hung
Trying this one more time

retest this please

bbejeck · 2020-08-15T16:28:09Z

merged #9108 into trunk

Thanks for the contribution @albert02lowis!

albert02lowis · 2020-08-15T16:45:04Z

Thanks for the reviews too @bbejeck and @abbccdda ! Really excited as this is my first contribution 🙏🏻

apache#9108) The main goal is to remove usage of embedded broker (EmbeddedKafkaCluster) in AbstractJoinIntegrationTest and its subclasses. This is because the tests under this class are no longer using the embedded broker, except for two. testShouldAutoShutdownOnIncompleteMetadata is one of such tests. Furthermore, this test does not actually perfom stream-table join; it is testing an edge case of joining with a non-existent topic, so it should be in a separate test. Testing strategy: run existing unit and integration test Reviewers: Boyang Chen <boyang@confluent.io>, Bill Bejeck <bbejeck@apache.org>

abbccdda added the streams label Jul 31, 2020

abbccdda reviewed Jul 31, 2020

View reviewed changes

albert02lowis force-pushed the albert_asdf branch from 48e3f83 to 1669509 Compare August 1, 2020 02:29

albert02lowis added 2 commits August 1, 2020 14:54

albert02lowis force-pushed the albert_asdf branch from 79f4c72 to 865379e Compare August 1, 2020 09:39

bbejeck reviewed Aug 12, 2020

View reviewed changes

bbejeck reviewed Aug 13, 2020

View reviewed changes

bbejeck merged commit 153db48 into apache:trunk Aug 15, 2020

albert02lowis deleted the albert_asdf branch August 16, 2020 06:06

Conversation

albert02lowis commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

abbccdda left a comment

Choose a reason for hiding this comment

Uh oh!

abbccdda commented Jul 31, 2020

Uh oh!

albert02lowis commented Aug 1, 2020

Uh oh!

abbccdda commented Aug 1, 2020

Uh oh!

albert02lowis commented Aug 3, 2020

Uh oh!

bbejeck commented Aug 12, 2020

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

bbejeck Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albert02lowis Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albert02lowis Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

bbejeck commented Aug 13, 2020

Uh oh!

bbejeck commented Aug 13, 2020

Uh oh!

bbejeck commented Aug 13, 2020

Uh oh!

abbccdda commented Aug 13, 2020

Uh oh!

bbejeck commented Aug 13, 2020

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

bbejeck commented Aug 14, 2020

Uh oh!

bbejeck commented Aug 14, 2020

Uh oh!

bbejeck commented Aug 14, 2020

Uh oh!

bbejeck commented Aug 15, 2020

Uh oh!

bbejeck commented Aug 15, 2020

Uh oh!

albert02lowis commented Aug 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

albert02lowis commented Jul 31, 2020 •

edited

Loading

bbejeck Aug 12, 2020 •

edited

Loading

albert02lowis Aug 13, 2020 •

edited

Loading