Skip to content

KAFKA-7536: Initialize TopologyTestDriver with non-null topic#5923

Merged
guozhangwang merged 4 commits intoapache:trunkfrom
guozhangwang:K7536-initialize-with-topic
Nov 20, 2018
Merged

KAFKA-7536: Initialize TopologyTestDriver with non-null topic#5923
guozhangwang merged 4 commits intoapache:trunkfrom
guozhangwang:K7536-initialize-with-topic

Conversation

@guozhangwang
Copy link
Copy Markdown
Contributor

In TopologyTestDriver constructor set non-null topic; and in unit test intentionally turn on caching to verify this case.

Committer Checklist (excluded from commit message)

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

@guozhangwang guozhangwang requested a review from mjsax November 17, 2018 22:22
);
globalStateTask.initialize();
globalProcessorContext.setRecordContext(new ProcessorRecordContext(0L, -1L, -1, null, new RecordHeaders()));
globalProcessorContext.setRecordContext(new ProcessorRecordContext(0, -1L, -1, ProcessorContextImpl.NONEXIST_TOPIC, new RecordHeaders()));
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.

Nit: why change 0L to 0 ? Timestamp is of type long. (sam below)

Stores.inMemoryKeyValueStore("aggStore"),
Serdes.String(),
Serdes.Long()),
Serdes.Long()).withCachingEnabled(), // intentionally turn on caching to achieve better test coverage
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.

I would prefer a separate test: shouldAllowPrePopulatingStatesStoresWithCachingEnabled()

@mjsax mjsax added the streams label Nov 18, 2018
@guozhangwang
Copy link
Copy Markdown
Contributor Author

@mjsax addressed comments.

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 for this @guozhangwang. LGTM

@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Nov 19, 2018

failures unrelated

restest this please

Copy link
Copy Markdown
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.

One nit.

LGTM. Feel free to merge.

Test timed out. Retest this please.

topology.addStateStore(Stores.keyValueStoreBuilder(
Stores.inMemoryKeyValueStore("aggStore"),
Serdes.String(),
Serdes.Long()).withCachingEnabled(), // intentionally turn on caching to achieve better test coverage
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.

nit: we can remove this comment, as the test name explains it :)

@guozhangwang guozhangwang merged commit 46e7b13 into apache:trunk Nov 20, 2018
guozhangwang added a commit that referenced this pull request Nov 20, 2018
In TopologyTestDriver constructor set non-null topic; and in unit test intentionally turn on caching to verify this case.

Reviewers: Bill Bejeck <bill@confluent.io>, Matthias J. Sax <matthias@confluent.io>
guozhangwang added a commit that referenced this pull request Nov 20, 2018
In TopologyTestDriver constructor set non-null topic; and in unit test intentionally turn on caching to verify this case.

Reviewers: Bill Bejeck <bill@confluent.io>, Matthias J. Sax <matthias@confluent.io>
@guozhangwang
Copy link
Copy Markdown
Contributor Author

I tried to cherry-pick to the earliest branch (1.1) but realized it has a few other diffs like KAFKA-6967 in the middle which are not cherry-picked either and hence making the divergence too large. Instead I'd only cherry-pick to 2.0 and 2.1.

pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
…#5923)

In TopologyTestDriver constructor set non-null topic; and in unit test intentionally turn on caching to verify this case.

Reviewers: Bill Bejeck <bill@confluent.io>, Matthias J. Sax <matthias@confluent.io>
@guozhangwang guozhangwang deleted the K7536-initialize-with-topic branch April 25, 2020 00:06
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.

3 participants