KAFKA-7420: Global store surrounded by read only implementation by nizhikov · Pull Request #5865 · apache/kafka

nizhikov · 2018-11-01T13:25:06Z

Global store surrounded by read-only KeyValueStore implementation.
All methods that try to write into Store will throw UnsupportedOperationException

Committer Checklist (excluded from commit message)

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

nizhikov · 2018-11-01T13:35:22Z

Hello, @mjsax! Can you take a look at my changes?

nizhikov · 2018-11-01T21:45:05Z

Failure unrelated.
Retest this, please

nizhikov · 2018-11-03T07:15:39Z

Failure unrelated.
Retest this, please

mjsax

Thanks for the PR @nizhikov! We should add a new ProcessorContextImplTest -- would be even better, if we test all existing methods, too, to close this testing gap.

nizhikov · 2018-11-04T07:56:29Z

Failure unrelated.
Retest this, please

mjsax · 2018-11-04T18:18:17Z

@nizhikov We stabilized a couple of test recently. Can you rebase this PR? This should help to get green builds.

nizhikov · 2018-11-04T19:59:21Z

@mjsax

We should add a new ProcessorContextImplTest

Please, tell me, what should be tested by this?

nizhikov · 2018-11-05T07:27:48Z

Failure unrelated.
Retest this, please

nizhikov · 2018-11-08T06:05:24Z

@mjsax

We should add a new ProcessorContextImplTest

Can you help me with this? What should be tested?

mjsax · 2018-11-08T23:51:06Z

We should test, that if a globalstore is returned from the context, calling any of the protected methods throws an exception.

nizhikov · 2018-11-16T06:28:01Z

@mjsax Test added. Please, review.

nizhikov · 2018-11-16T19:30:18Z

Tests passed

nizhikov · 2018-11-20T17:32:45Z

@mjsax Can you take a look at my changes?

mjsax · 2018-11-20T22:34:54Z

Call for second review. Anyof @guozhangwang @bbejeck @vvcephei

bbejeck

Thanks for the contribution @nizhikov!

Overall looks good, but I think we should get test coverage on the non-exception throwing methods from the KeyValueStore wrapper of the global store.

nizhikov · 2018-11-21T04:21:38Z

@bbejeck Thank you for the review.

I've added tests for non-exception methods of KeyValueStore.
Please, take a look.

mjsax · 2018-11-21T17:47:10Z

Build failed with a checkstyle error

bbejeck

@nizhikov thanks for the updated tests, LGTM pending the checkstyle fix

nizhikov · 2018-11-21T18:43:17Z

@mjsax
@bbejeck

Thanks for the review.

Build failed with a checkstyle error
LGTM pending the checkstyle fix

🤦‍♂️ Sorry about that. Fixed

nizhikov · 2018-11-21T21:07:44Z

Tests passed. Can we merge this PR?

mjsax

I was just about to merge this PR, but I suddenly have doubts about the correctness. For global stores, we do not dictate that it must be a KeyValueStore (only for GlobalKTables) if I am not wrong.

Thus, if somebody uses a custom store, the cast into a KeyValueStore in L79 would fail and we would crash.

For library provided stores, it's only possible to use a KeyValueStore though (WindowStore and SessionStores are not supported for the provided default implementations). However, users could also use custom implementation of WindowStore or SessionStore -- the problem is not with the interfaces, but with the implementation of the or built-in window/session store.

Thus, I think, we need to check the type of the global store, and wrap it accordingly if it implements one of the three store interfaces. The store type might also so unknown, and for this case we need to return the store as-is.

Does this make sense?

It would be great to add test for all those cases.

nizhikov · 2018-11-22T19:10:24Z

Thus, I think, we need to check the type of the global store, and wrap it accordingly if it implements one of the three store interfaces.

We should wrap in read only decorator instances that implements 3 following interfaces:

KeyValueStore
WindowStore
SessionStore

@mjsax , am I understand you correctly?

I will provide this fix and tests shortly.

mjsax · 2018-11-23T06:34:02Z

Yes. Check the type, for KeyValueStore do what the current PR does, and for Window/Session-Store apply the same pattern.

nizhikov · 2018-11-23T17:29:26Z

@mjsax I've implement your proposal. Please, review.

nizhikov · 2018-11-24T06:52:58Z

Tests are green.

mjsax

Thanks fo the update! Couple of nits.

Call for review @guozhangwang

nizhikov · 2018-11-27T03:57:18Z

Thanks for the update! Couple of nits.

Nits fixed.

nizhikov · 2018-11-27T14:22:47Z

Tests are green.

nizhikov · 2018-11-28T03:32:12Z

Hello, @guozhangwang

Please, review and my merge my PR.

I have 2 approval from mjsax and bbejeck

nizhikov · 2018-11-28T03:33:00Z

@mjsax Do I need to improve this PR? Can you approve it?

nizhikov · 2018-11-29T07:17:57Z

Hello @mjsax
I got 2 OKs from you and bbejeck
Do I need one more review from guozhangwang?

No rush, I just want to know, how it's work in Kafka community

Can you merge this PR by yourself? :)

mjsax · 2018-11-30T01:45:40Z

@nizhikov Committers can merge PRs.

I would like to get a confirmation from @bbejeck because we did some mayor changes after he approved. (Or a second review from @guozhangwang @vvcephei).

We want two +1 before merging.

bbejeck

I took another pass over the updates LGTM

nizhikov · 2018-11-30T06:56:06Z

@bbejeck Thank you.

@mjsax There are two +1. Let's merge it?! :)

mjsax · 2018-11-30T19:44:14Z

SGTM. There was a merge into trunk enabling spotBugs -- can you rebase this PR one more time to make sure the build is green (just as a guard to not break trunk if we merge). Thanks.

…rator on return.

nizhikov · 2018-11-30T19:50:13Z

can you rebase this PR one more time to make sure the build is green (just as a guard to not break trunk if we merge). Thanks.

Done

nizhikov · 2018-12-01T08:21:22Z

@mjsax

just as a guard to not break trunk if we merge

Tests are green.

nizhikov · 2018-12-03T15:51:50Z

Hello, @mjsax
Is there anything that should be improved prior merge?

nizhikov · 2018-12-05T15:17:57Z

Hello, @mjsax

Do you have a change do look at the tests results?
Can we merge PR?

Should I ask Bill Bejeck or Guozhang Wang to review and merge this patch?

mjsax · 2018-12-05T19:27:04Z

Thanks for the PR @nizhikov! Merged to trunk.

Please be a little patient. We try to get back to PRs as soon as we can, but it might take couple of days. :)

nizhikov · 2018-12-05T20:05:46Z

Thank you, @mjsax

Sorry for pinging you too many times.
WIll try to avoid it in the next contributions.

@kamalcph

…he#5865) Reviewers: Matthias J. Sax <matthias@confluent.io>, Kamal Chandraprakash (@kamalcph), Bill Bejeck <bill@confluent.io>

mjsax added the streams label Nov 1, 2018

mjsax reviewed Nov 4, 2018

View reviewed changes

Comment thread streams/src/main/java/org/apache/kafka/streams/processor/internals/ProcessorContextImpl.java Outdated

nizhikov force-pushed the KAFKA-7420 branch from b217d99 to 67a79fa Compare November 4, 2018 18:48

kamalcph reviewed Nov 5, 2018

View reviewed changes

Comment thread streams/src/main/java/org/apache/kafka/streams/processor/internals/ProcessorContextImpl.java Outdated

nizhikov force-pushed the KAFKA-7420 branch from 57ee8e8 to 60aabed Compare November 7, 2018 04:16

bbejeck reviewed Nov 20, 2018

View reviewed changes

bbejeck approved these changes Nov 21, 2018

View reviewed changes

mjsax reviewed Nov 22, 2018

View reviewed changes

Comment thread streams/src/main/java/org/apache/kafka/streams/processor/internals/ProcessorContextImpl.java Outdated

mjsax reviewed Nov 27, 2018

View reviewed changes

mjsax approved these changes Nov 28, 2018

View reviewed changes

bbejeck approved these changes Nov 30, 2018

View reviewed changes

nizhikov added 9 commits November 30, 2018 22:47

KAFKA-7420: Global store surrounded by read only implementation

8c495cd

KAFKA-7420: flush throws.

6d3a248

KAFKA-7420: test added.

f219a11

KAFKA-7420: test added

fae9e9d

KAFKA-7420: formatting fix

2dd9324

KAFKA-7420: positive tests added

50f887d

KAFKA-7420: code style fixes

2db3c2a

KAFKA-7420: All possible global stores surrounded with read only deco…

b6a4556

…rator on return.

KAFKA-7420: code style fixes

e9dfdb2

nizhikov force-pushed the KAFKA-7420 branch from 1e5353c to e9dfdb2 Compare November 30, 2018 19:49

mjsax merged commit ec501f3 into apache:trunk Dec 5, 2018

nizhikov mentioned this pull request Dec 9, 2018

KAFKA-6970: All standard state stores guarded with read only wrapper #6016

Merged

3 tasks

Conversation

nizhikov commented Nov 1, 2018

Committer Checklist (excluded from commit message)

Uh oh!

nizhikov commented Nov 1, 2018

Uh oh!

nizhikov commented Nov 1, 2018

Uh oh!

nizhikov commented Nov 3, 2018

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nizhikov commented Nov 4, 2018

Uh oh!

mjsax commented Nov 4, 2018

Uh oh!

nizhikov commented Nov 4, 2018

Uh oh!

nizhikov commented Nov 5, 2018

Uh oh!

Uh oh!

nizhikov commented Nov 8, 2018

Uh oh!

mjsax commented Nov 8, 2018

Uh oh!

nizhikov commented Nov 16, 2018

Uh oh!

nizhikov commented Nov 16, 2018

Uh oh!

nizhikov commented Nov 20, 2018

Uh oh!

mjsax commented Nov 20, 2018

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

nizhikov commented Nov 21, 2018

Uh oh!

mjsax commented Nov 21, 2018

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

nizhikov commented Nov 21, 2018

Uh oh!

nizhikov commented Nov 21, 2018

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nizhikov commented Nov 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjsax commented Nov 23, 2018

Uh oh!

nizhikov commented Nov 23, 2018

Uh oh!

nizhikov commented Nov 24, 2018

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nizhikov commented Nov 27, 2018

Uh oh!

nizhikov commented Nov 27, 2018

Uh oh!

nizhikov commented Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nizhikov commented Nov 28, 2018

Uh oh!

nizhikov commented Nov 29, 2018

nizhikov commented Nov 22, 2018 •

edited

Loading

nizhikov commented Nov 28, 2018 •

edited

Loading

nizhikov commented Dec 1, 2018 •

edited

Loading