Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-6474: Rewrite tests to use new public TopologyTestDriver [part 3] #5052

Merged
merged 2 commits into from
May 22, 2018

Conversation

h314to
Copy link
Contributor

@h314to h314to commented May 21, 2018

This PR is a further step towards the complete replacement of KStreamTestDriver with TopologyTestDriver. These straightforward changes were split from another PR to simplify the review process.

  • Refactor:
    • KStreamWindowReduceTest
    • KTableMapKeysTest
    • SessionWindowedKStreamImplTest
    • TimeWindowedKStreamImplTest

* Refactor:
  - KStreamWindowReduceTest
  - KTableMapKeysTest
  - SessionWindowedKStreamImplTest
  - TimeWindowedKStreamImplTest
@mjsax
Copy link
Member

mjsax commented May 21, 2018

\cc @bbejeck @vvcephei

Copy link
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the PR!

Waiting for second review and Jenkins to pass before merging.

Copy link
Contributor

@guozhangwang guozhangwang left a comment

Choose a reason for hiding this comment

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

Left a couple comments on checking @SuppressWarnings("unchecked"). Otherwise LGTM!

processData();
final SessionStore<String, String> sessionStore = (SessionStore<String, String>) driver.allStateStores().get("reduced");
final List<KeyValue<Windowed<String>, String>> data = StreamsTestUtils.toList(sessionStore.fetch("1", "2"));
try (final TopologyTestDriver driver = new TopologyTestDriver(builder.build(), props, 0L)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think some of the above @SuppressWarnings("unchecked") can be removed now as well; could you double check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they can. Thank for catching this.

processData();
final WindowStore<String, String> windowStore = (WindowStore<String, String>) driver.allStateStores().get("reduced");
final List<KeyValue<Windowed<String>, String>> data = StreamsTestUtils.toList(windowStore.fetch("1", "2", 0, 1000));
try (final TopologyTestDriver driver = new TopologyTestDriver(builder.build(), props, 0L)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto here for @SuppressWarnings("unchecked") above?

@bbejeck
Copy link
Contributor

bbejeck commented May 22, 2018

retest this please

Copy link
Contributor

@bbejeck bbejeck left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! LGTM.

@h314to
Copy link
Contributor Author

h314to commented May 22, 2018

I keep getting an (unrelated) timeout error on kafka.server.DynamicBrokerReconfigurationTest.testUncleanLeaderElectionEnable with JDK10 and Scala 2.12

@guozhangwang
Copy link
Contributor

@h314to This is a known issue and is being fixed.

@guozhangwang guozhangwang merged commit 6281fbc into apache:trunk May 22, 2018
@guozhangwang
Copy link
Contributor

Merged to trunk, thanks @h314to !

@h314to
Copy link
Contributor Author

h314to commented May 22, 2018

Great! :)

@h314to h314to deleted the fix/KAFKA-6474-part3 branch May 22, 2018 15:54
ying-zheng pushed a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018
…3] (apache#5052)

* KAFKA-6474: Rewrite tests to use new public TopologyTestDriver [part 3]

* Refactor:
  - KStreamWindowReduceTest
  - KTableMapKeysTest
  - SessionWindowedKStreamImplTest
  - TimeWindowedKStreamImplTest

* Remove unnecessary @SuppressWarnings(unchecked)

Reviewers: Bill Bejeck <bill@confluent.io>, Matthias J. Sax <matthias@confluent.io>, Guozhang Wang <wangguoz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants