Skip to content

KAFKA-8011: Fix flaky RegexSourceIntegrationTest#8799

Merged
mjsax merged 3 commits into
apache:trunkfrom
mjsax:kafka-8011-RegexSourceIntegrationTest
Jun 5, 2020
Merged

KAFKA-8011: Fix flaky RegexSourceIntegrationTest#8799
mjsax merged 3 commits into
apache:trunkfrom
mjsax:kafka-8011-RegexSourceIntegrationTest

Conversation

@mjsax

@mjsax mjsax commented Jun 4, 2020

Copy link
Copy Markdown
Member

Ensure that tests cleanup after themselves and use different app.id per test run.

Call for review @guozhangwang @ableegoldman

ableegoldman
ableegoldman approved these changes Jun 4, 2020
View reviewed changes

@ableegoldman ableegoldman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One question, but overall LGTM. Thanks!

Comment thread streams/src/test/java/org/apache/kafka/streams/integration/RegexSourceIntegrationTest.java

streams.close();
CLUSTER.deleteTopicsAndWait("TEST-TOPIC-1", "TEST-TOPIC-2");
streams.close();

@ableegoldman ableegoldman Jun 4, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we call close in the finally block? Here and elsewhere

@mjsax mjsax Jun 4, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think that is necessary -- there is an @After method that closed the client for us.

@ableegoldman ableegoldman Jun 4, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Then why close it here as well?

@mjsax mjsax Jun 4, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think it's better to first close it before we delete the topics.

@ableegoldman ableegoldman Jun 4, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well, won't we end up deleting the topics before closing it if we never reach the first streams.close ? Or does it not really matter in that case since something has already gone wrong (just curious, I'm fine with it as-is btw)

@mjsax mjsax Jun 4, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, that is my reasoning.

@guozhangwang guozhangwang Jun 5, 2020

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.

Why we need to call streams.close() inside the function given they are always called in tearDown? Ditto below.

@mjsax mjsax Jun 5, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

As mentioned above: we should close the client before we delete the input topics. -- Seems cleaner.

guozhangwang
guozhangwang reviewed Jun 5, 2020
View reviewed changes
Comment thread streams/src/test/java/org/apache/kafka/streams/integration/RegexSourceIntegrationTest.java Outdated
properties.put(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, "earliest");

streamsConfiguration = StreamsTestUtils.getStreamsConfig("regex-source-integration-test",
streamsConfiguration = StreamsTestUtils.getStreamsConfig("regex-source-integration-test-" + topicSuffixGenerator.get(),

@guozhangwang guozhangwang Jun 5, 2020

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.

Could we use

    @Rule
    public TestName testName = new TestName();

instead as the suffix?

Comment thread streams/src/test/java/org/apache/kafka/streams/integration/RegexSourceIntegrationTest.java

streams.close();
CLUSTER.deleteTopicsAndWait("TEST-TOPIC-1", "TEST-TOPIC-2");
streams.close();

@guozhangwang guozhangwang Jun 5, 2020

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.

Why we need to call streams.close() inside the function given they are always called in tearDown? Ditto below.

@guozhangwang

guozhangwang commented Jun 5, 2020

Copy link
Copy Markdown
Contributor

LGTM.

@mjsax
mjsax merged commit 5a0e65e into apache:trunk Jun 5, 2020
@mjsax
mjsax deleted the kafka-8011-RegexSourceIntegrationTest branch June 5, 2020 21:38
Kvicii pushed a commit to Kvicii/kafka that referenced this pull request Jun 6, 2020
Merge branch 'trunk' of github.com:apache/kafka into trunk-read
6f039a3 
* 'trunk' of github.com:apache/kafka: (46 commits)
  MINOR: improve code encapsulation between StreamThread and TaskManager (apache#8819)
  Fixing KAFKA-10094 (apache#8797)
  KAFKA-9851: Revoking Connect tasks due to connectivity issues should also clear the running assignment (apache#8804)
  KAFKA-9840; Skip End Offset validation when the leader epoch is not reliable (apache#8486)
  HOT_FIX: Update javadoc since imports added (apache#8817)
  KAFKA-8011: Fix flaky RegexSourceIntegrationTest (apache#8799)
  KAFKA-9570: Define SSL configs in all worker config classes, not just distributed (apache#8135)
  KAFKA-10111: Make SinkTaskContext.errantRecordReporter() a default method (apache#8814)
  KAFKA-10110: Corrected potential NPE when null label value added to KafkaMetricsContext (apache#8811)
  MINOR: Change the order that Connect calls `config()` and `validate()` to avoid validating if the required ConfigDef is null (apache#8810)
  MINOR: fix backwards incompatibility in JmxReporter introduced by KIP-606
  MINOR: Fix javadoc warnings (apache#8809)
  KAFKA-9441: Improve Kafka Streams task management (apache#8776)
  fix the broken links of streams javadoc (apache#8789)
  KAFKA-10040; Make computing the PreferredReplicaImbalanceCount metric more efficient (apache#8724)
  KAFKA-10066: TestOutputTopic should pass record headers into deserializers (apache#8759)
  MINOR: Add explanation for disabling forwarding from value transformers (apache#8771)
  KAFKA-10033: Throw UnknownTopicOrPartitionException if altering configs of non-existing topic
  KAFKA-9434: automated protocol for alterReplicaLogDirs (apache#8311)
  KAFKA-9313: Set `use_all_dns_ips` as the new default for `client.dns.lookup` (KIP-602) (apache#8644)
  ...
ijuma added a commit to confluentinc/kafka that referenced this pull request Jun 8, 2020
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into ccs-master
a6f935e 
Conflicts:
* build.gradle: take upstream changes regarding heap memory
configuration for the build.

* apache-github/trunk: (33 commits)
  MINOR: fix HTML markup (apache#8823)
  KAFKA-10012; Reduce overhead of strings in SelectorMetrics (apache#8684)
KAFKA-9216: Enforce internal config topic settings for Connect workers
during startup (apache#8270)
  KAFKA-10097: Internalize checkpoint data (apache#8820)
KAFKA-10033: Throw UnknownTopicOrPartitionException when modifying a
non-existent topic's config
MINOR: improve code encapsulation between StreamThread and TaskManager
(apache#8819)
  Fixing KAFKA-10094 (apache#8797)
KAFKA-9851: Revoking Connect tasks due to connectivity issues should
also clear the running assignment (apache#8804)
KAFKA-9840; Skip End Offset validation when the leader epoch is not
reliable (apache#8486)
  HOT_FIX: Update javadoc since imports added (apache#8817)
  KAFKA-8011: Fix flaky RegexSourceIntegrationTest (apache#8799)
KAFKA-9570: Define SSL configs in all worker config classes, not just
distributed (apache#8135)
KAFKA-10111: Make SinkTaskContext.errantRecordReporter() a default
method (apache#8814)
KAFKA-10110: Corrected potential NPE when null label value added to
KafkaMetricsContext (apache#8811)
MINOR: Change the order that Connect calls `config()` and `validate()`
to avoid validating if the required ConfigDef is null (apache#8810)
MINOR: fix backwards incompatibility in JmxReporter introduced by
KIP-606
  MINOR: Fix javadoc warnings (apache#8809)
  KAFKA-9441: Improve Kafka Streams task management (apache#8776)
  fix the broken links of streams javadoc (apache#8789)
KAFKA-10040; Make computing the PreferredReplicaImbalanceCount metric
more efficient (apache#8724)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ableegoldman ableegoldman ableegoldman approved these changes
@guozhangwang guozhangwang guozhangwang left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mjsax @guozhangwang @ableegoldman