KAFKA-7075: Allow Topology#addGlobalStore to add a window store by lmontrieux · Pull Request #5725 · apache/kafka

lmontrieux · 2018-10-02T09:24:21Z

InternalTopologyBuilder#addGlobalStore has no tests for stores other than KeyValueStore. This PR adds a new unit test to show that a StoreBuilder can be used in InternalTopologyBuilder#addGlobalStore. This required creating two new mocks, MockWindowStore and MockWindowStoreBuilder, modelled on MockKeyValueStore and MockKeyValueStoreBuilder, respectively.

Committer Checklist (excluded from commit message)

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

lmontrieux · 2018-10-02T09:25:24Z

@guozhangwang since you reported the bug in Jira, would you like to have a look at this PR?

lmontrieux · 2018-10-03T09:51:21Z

retest this please

mjsax

Thanks for the PR. Some comments.

mjsax · 2018-10-04T00:07:03Z

Does the cast make sense? addGlobalStore has not generic type any more and thus the type cannot be checked anyway?

I guess you are right. The cast was useful when addGlobalStore still had a generic type, as it would refuse to compile. I should have removed it.

@mjsax fixed it in 24f1fc3

mjsax · 2018-10-04T00:10:36Z

Do we need MockWindowStoreBuilder and MockWindowStore ? Can't we just use Mockito ?

We could use Mockito. I went for EasyMock as there were already MockKeyValueStoreBuilder and MockKeyValueStore, so it seemed like a nice fit, at least for someone who, like me, is only getting started with the Kafka code. I since saw discussions about using Mockito instead of EasyMock on the mailing list, and I agree that it makes sense to use Mockito.

Actually I can't seem to find any use of mockito anywhere in this repo, so perhaps it is better to stick with easymock for this PR? What do you think?

That's fine -- it's just about avoiding own mock classes.

bbejeck

@lmontrieux thanks for the contribution.

In my mind, one of the reasons the addGlobalStore method accepts a KeyValueStore is Streams doesn't need to know anything about the data since the data from the backing topic is in key-value pairs already. So loading the global store is just a matter of put(key, value).

Can discuss how the loading of a windowed GlobalStore would work? Maybe provide an integration test as well?

mjsax · 2018-10-04T22:41:27Z

@bbejeck Note, that when adding a global store, user also provides the corresponding processor. This method is at Processor API level and it's the users responsibility to implement the Processor correctly. At DSL level, the API is still limited to accept KeyValueStore only.

Adding an integration test is always a good idea of course :)

bbejeck · 2018-10-04T22:55:46Z

that when adding a global store, user also provides the corresponding processor.

@mjsax hhanks for the clarification. As soon as I read your comment I realized that I knew that, but it slipped my mind at the moment.

Personal confusion aside, I agree with you that an integration test is still worthwhile.

lmontrieux · 2018-10-05T06:36:29Z

@mjsax @bbejeck thank you for your comments. I will add an integration test asap.

bbejeck · 2018-11-19T17:41:10Z

Hi, @lmontrieux are you still working on this PR?

lmontrieux · 2018-11-19T18:44:49Z

Hi @bbejeck ,
Sorry for the delay, life got in the way. I expect to give this PR some attention over the weekend.

bbejeck · 2018-11-27T16:53:24Z

Hi @lmontrieux no need to apologize! Just checking in and thanks again for contributing!

mjsax · 2019-02-21T01:09:30Z

@lmontrieux Are you still interested in working on this?

guozhangwang · 2019-02-22T01:16:27Z

Actually, looking at the latest trunk, I think the enforced types have already been resolved, but we still need this PR for adding the shouldAllowAddGlobalStoreWithNonKVStore test to cover this call path.

@lmontrieux could you rebase your PR and then only leave the test / mocks in place, and run them to see if they pass? If yes, we can go ahead and review / merge this PR still.

lmontrieux · 2019-02-25T20:05:09Z

@guozhangwang sure, I'll do that

guozhangwang · 2019-02-25T21:07:12Z

Thank you!

lmontrieux · 2019-02-26T20:46:38Z

Wow, I did something wrong. Will fix it.

…com:lmontrieux/kafka into KAFKA-7075-addGlobalStore-with-window-store

lmontrieux · 2019-02-26T22:49:39Z

Some warnings are causing the build to fail, I'll have a look

mjsax · 2019-03-06T00:29:20Z

@lmontrieux Any updates on this PR?

lmontrieux · 2019-03-11T06:21:48Z

@mjsax now all tests are passing. I'll update the PR description to reflect that this only adds unit tests.

bbejeck

Thanks @lmontrieux! LGTM.

bbejeck · 2019-03-11T15:33:02Z

\cc @mjsax for a second review

mjsax · 2019-03-14T18:45:24Z

The original ticket claims, that it's not possible to add a non-kv-store as global store? This PR adds test that proof the ticket wrong? Or was it fixed with a different PR?

mjsax · 2019-03-14T18:47:56Z

Seems this was fixed via #5779

Should we close the ticket as "contained by" and merge this as MINOR PR instead?

mjsax · 2019-03-14T18:52:24Z

+    @Test
+    public void shouldAllowAddGlobalStoreWithNonKVStore() {
+        final StoreBuilder windowStoreBuilder = new MockWindowStoreBuilder("store", false);
+        builder.addGlobalStore(windowStoreBuilder.withLoggingDisabled(),


The signature of addGlobalStore accepts any StoreBuilder, thus I am wondering if we should switch to pass in a TestStateStoreBuilder that returns StateStore instead of a WindowedStore?

I am also thinking if we should add an test using TopologyTestDriver to verify that a TestStateStore can be used as global store?

Makes sense, I'll update the PR accordingly

bbejeck · 2019-03-20T20:59:22Z

Should we close the ticket as "contained by" and merge this as MINOR PR instead?

SGTM.

@lmontrieux can you respond to @mjsax's comments?

lmontrieux · 2019-03-21T08:15:47Z

Seems this was fixed via #5779

Should we close the ticket as "contained by" and merge this as MINOR PR instead?

Yes, makes sense to me.

bbejeck · 2019-04-17T16:55:29Z

@lmontrieux what's status of this PR? Once you address the latest comments we can get this merged. Thanks!

mjsax · 2019-06-01T05:42:22Z

@lmontrieux -- just cycling back to this PR. Are you still interesting? Seems the PR would need a rebase and there a some open comments to address.

mjsax · 2022-12-29T00:32:51Z

Closing this old PR due to inactivity.

fonsdant · 2024-11-20T18:19:48Z

@mjsax, do you think this PR is still needed? If yes, and if @lmontrieux agrees, I could assume this work.

mjsax · 2025-01-04T00:51:54Z

@fonsdant I am not 100% sure -- guess it should not be too hard to write a small unit test for it (or first check if one might already exist)? If the unit test passed, we know it's not an issue any longer (and we could close some testing gap).

If it's still a problem feel free to pick it up. This PR was close 2 years ago, and nobody is working on this.

mjsax added the streams label Oct 4, 2018

mjsax reviewed Oct 4, 2018

View reviewed changes

bbejeck reviewed Oct 4, 2018

View reviewed changes

lmontrieux force-pushed the KAFKA-7075-addGlobalStore-with-window-store branch from f7b368b to d92ab5f Compare February 26, 2019 20:48

lmontrieux added 3 commits February 26, 2019 21:49

Pass any StoreBuilder to InternalTopologyBuilder#addGlobalStore

dcebf75

Remove unnecessary cast

2549af3

Clean imports

37a287a

lmontrieux force-pushed the KAFKA-7075-addGlobalStore-with-window-store branch from d92ab5f to 37a287a Compare February 26, 2019 20:49

lmontrieux added 4 commits February 26, 2019 23:28

Pass any StoreBuilder to InternalTopologyBuilder#addGlobalStore

dafb1ef

Remove unnecessary cast

c04f455

Clean imports

0d0c475

Merge branch 'KAFKA-7075-addGlobalStore-with-window-store' of github.…

abe2524

…com:lmontrieux/kafka into KAFKA-7075-addGlobalStore-with-window-store

Suppress deprecation warnings in MockWindowStore

5f31cff

bbejeck approved these changes Mar 11, 2019

View reviewed changes

mjsax reviewed Mar 14, 2019

View reviewed changes

andrewchoi5 approved these changes Feb 7, 2020

View reviewed changes

mjsax closed this Dec 29, 2022

Conversation

lmontrieux commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

lmontrieux commented Oct 2, 2018

Uh oh!

lmontrieux commented Oct 3, 2018

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

mjsax commented Oct 4, 2018

Uh oh!

bbejeck commented Oct 4, 2018

Uh oh!

lmontrieux commented Oct 5, 2018

Uh oh!

bbejeck commented Nov 19, 2018

Uh oh!

lmontrieux commented Nov 19, 2018

Uh oh!

bbejeck commented Nov 27, 2018

Uh oh!

mjsax commented Feb 21, 2019

Uh oh!

guozhangwang commented Feb 22, 2019

Uh oh!

lmontrieux commented Feb 25, 2019

Uh oh!

guozhangwang commented Feb 25, 2019

Uh oh!

lmontrieux commented Feb 26, 2019

Uh oh!

lmontrieux commented Feb 26, 2019

Uh oh!

mjsax commented Mar 6, 2019

Uh oh!

lmontrieux commented Mar 11, 2019

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

bbejeck commented Mar 11, 2019

Uh oh!

mjsax commented Mar 14, 2019

Uh oh!

mjsax commented Mar 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbejeck commented Mar 20, 2019

Uh oh!

lmontrieux commented Mar 21, 2019

Uh oh!

bbejeck commented Apr 17, 2019

Uh oh!

mjsax commented Jun 1, 2019

Uh oh!

mjsax commented Dec 29, 2022

Uh oh!

lmontrieux commented Oct 2, 2018 •

edited

Loading