Skip to content

KAFAK-3522: Add TopologyTestDriver unit tests#6179

Merged
mjsax merged 4 commits intoapache:trunkfrom
mjsax:kafka-3522-rocksdb-format-topology-test-driver-tests
May 16, 2019
Merged

KAFAK-3522: Add TopologyTestDriver unit tests#6179
mjsax merged 4 commits intoapache:trunkfrom
mjsax:kafka-3522-rocksdb-format-topology-test-driver-tests

Conversation

@mjsax
Copy link
Copy Markdown
Member

@mjsax mjsax commented Jan 20, 2019 •
edited
Loading

Part of KIP-258.

@mjsax mjsax added the streams label Jan 20, 2019
@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Mar 21, 2019

@mjsax any updates on this PR as #6175 has been merged.

@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Mar 22, 2019

Need to find some time to rebase this PR... Will ping you when it's ready. Thanks for following up.

@mjsax mjsax force-pushed the kafka-3522-rocksdb-format-topology-test-driver-tests branch from 9d41589 to cf2f19e Compare May 1, 2019 23:48
mjsax
mjsax commented May 1, 2019
View reviewed changes
Comment thread streams/src/main/java/org/apache/kafka/streams/state/Stores.java Outdated
Copy link
Copy Markdown
Member Author

@mjsax mjsax May 1, 2019

Choose a reason for hiding this comment

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

Just reordering to "correct" position in alignment to the order of the other method.

mjsax
mjsax commented May 1, 2019
View reviewed changes
Comment thread streams/test-utils/src/test/java/org/apache/kafka/streams/TopologyTestDriverTest.java Outdated
Copy link
Copy Markdown
Member Author

@mjsax mjsax May 1, 2019

Choose a reason for hiding this comment

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

Java8 cleanup

@mjsax mjsax changed the title KAFAK-3522: add unit test for TopologyTestDriver [WIP] KAFAK-3522: Ensure Interactive Queries return correct store May 1, 2019
@mjsax mjsax changed the title KAFAK-3522: Ensure Interactive Queries return correct store KAFAK-3522: Add TopologyTestDriver unit tests May 2, 2019
mjsax
mjsax commented May 2, 2019
View reviewed changes
Comment thread streams/src/main/java/org/apache/kafka/streams/state/internals/GlobalStateStoreProvider.java Outdated
Copy link
Copy Markdown
Member Author

@mjsax mjsax May 2, 2019

Choose a reason for hiding this comment

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

Note, change is actually part of #6661 and just added here to make the tests pass -- #6661 should be merged first.

mjsax
mjsax commented May 2, 2019
View reviewed changes
Comment thread ...s/src/main/java/org/apache/kafka/streams/state/internals/StreamThreadStateStoreProvider.java Outdated
Copy link
Copy Markdown
Member Author

@mjsax mjsax May 2, 2019

Choose a reason for hiding this comment

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

Note, change is actually part of #6661 and just added here to make the tests pass -- #6661 should be merged first.

@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented May 2, 2019

Java11 failed with env issue. Java8 passed. Not triggering retest as Java11 seems to be broken.

vvcephei
vvcephei approved these changes May 6, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@vvcephei vvcephei left a comment

Choose a reason for hiding this comment

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

Thanks, @mjsax !

Comment thread streams/src/test/java/org/apache/kafka/streams/integration/StoreUpgradeIntegrationTest.java Outdated
Copy link
Copy Markdown
Contributor

@vvcephei vvcephei May 6, 2019

Choose a reason for hiding this comment

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

nit: I'm having a hard time maintaining a clear mental picture of what the expected result looks like as this test progresses. Can you consider using a literal list for each processKeyValueAndVerifyPlainCount instead?

@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented May 6, 2019

Updated this

vvcephei
vvcephei approved these changes May 6, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@vvcephei vvcephei left a comment

Choose a reason for hiding this comment

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

Thanks, @mjsax , I know that was a bunch of monotonous work. It's so much clearer to read now!

bbejeck
bbejeck reviewed May 7, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@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 @mjsax, just a couple of minor comments, otherwise LGTM

Comment thread ...c/test/java/org/apache/kafka/streams/state/internals/StreamThreadStateStoreProviderTest.java Outdated
Copy link
Copy Markdown
Member

@bbejeck bbejeck May 7, 2019

Choose a reason for hiding this comment

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

nit: do we want to add an assertion assertThat(store, instanceOf(TimestampedKeyValueStore.class)) here?

Comment thread ...c/test/java/org/apache/kafka/streams/state/internals/StreamThreadStateStoreProviderTest.java Outdated
Copy link
Copy Markdown
Member

@bbejeck bbejeck May 7, 2019

Choose a reason for hiding this comment

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

nit: do we want to add an assertion
assertThat(store, not(instanceOf(TimestampedKeyValueStore.class))); here?

Comment thread ...c/test/java/org/apache/kafka/streams/state/internals/StreamThreadStateStoreProviderTest.java Outdated
Copy link
Copy Markdown
Member

@bbejeck bbejeck May 7, 2019

Choose a reason for hiding this comment

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

typo in the test name? Looks to me it should be shouldNotFindTimestampedKeyValueStores

Copy link
Copy Markdown
Member Author

@mjsax mjsax May 8, 2019

Choose a reason for hiding this comment

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

The test name is actually correct, but the test is broken in this version. Rebasing based on #6661 fixed the issue.

@mjsax mjsax force-pushed the kafka-3522-rocksdb-format-topology-test-driver-tests branch from a06f488 to a13bb7d Compare May 8, 2019 23:14
@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented May 8, 2019

Rebase to resolve merge conflicts. Also address latest Github comments. Added new test for in-memory store upgrades. Those fail atm, exposing the bug as fixed in #6667 (comment)

Jenkins is expected to fail -- need to rebase this after #6667 is merged

@mjsax mjsax mentioned this pull request May 8, 2019
Merged
@guozhangwang
Copy link
Copy Markdown
Contributor

guozhangwang commented May 11, 2019

retest this please

@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented May 12, 2019

@guozhangwang I need to rebase this first... Test are expected to fail atm.

@mjsax mjsax force-pushed the kafka-3522-rocksdb-format-topology-test-driver-tests branch from a13bb7d to 846ecb4 Compare May 13, 2019 23:49
@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented May 13, 2019

Rebased this. Call for review.

@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented May 14, 2019

Java 8 passed, Java 11 failed with integration.kafka.api.ConsumerTopicCreationTest.testAutoTopicCreation[brokerTopicCreation=true, consumerTopicCreation=true

retest this please

bbejeck
bbejeck approved these changes May 14, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@bbejeck bbejeck left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @mjsax just one minor comment/question otherwise LGTM

Comment thread streams/test-utils/src/test/java/org/apache/kafka/streams/TopologyTestDriverTest.java
final String timestampedWindowStoreName = "windowTimestampStore";
final String sessionStoreName = "sessionStore";
final String globalKeyValueStoreName = "globalKeyValueStore";
final String globalTimestampedKeyValueStoreName = "globalKeyValueTimestampStore";
Copy link
Copy Markdown
Member

@bbejeck bbejeck May 14, 2019

Choose a reason for hiding this comment

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

one small nit: should this test include in-memory stores?

Copy link
Copy Markdown
Member Author

@mjsax mjsax May 15, 2019

Choose a reason for hiding this comment

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

Sure. Why not.

@guozhangwang
Copy link
Copy Markdown
Contributor

guozhangwang commented May 15, 2019

retest this please

@mjsax mjsax merged commit 16b4088 into apache:trunk May 16, 2019
@mjsax mjsax deleted the kafka-3522-rocksdb-format-topology-test-driver-tests branch May 16, 2019 15:16
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
@mjsax @pengxiaolong
KAFAK-3522: Add TopologyTestDriver unit tests (apache#6179)
9ad6a7e 
Reviewers: John Roesler <john@confluent.io>, Bill Bejeck <bill@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbejeck bbejeck bbejeck approved these changes

@vvcephei vvcephei vvcephei approved these changes

Assignees

No one assigned

Labels

streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mjsax @bbejeck @guozhangwang @vvcephei