KAFKA-7918: Inline generic parameters Pt. I: in-memory key-value store by ableegoldman · Pull Request #6293 · apache/kafka

ableegoldman · 2019-02-20T00:33:27Z

First PR in series to inline the generic parameters of the following bytes stores:

[x] InMemoryKeyValueStore
[ ] RocksDBWindowStore
[ ] RocksDBSessionStore
[ ] MemoryLRUCache
[ ] MemoryNavigableLRUCache
[ ] (awaiting merge) InMemoryWindowStore

A number of tests took advantage of the generic InMemoryKeyValueStore and had to be reworked somewhat -- this PR covers everything related to the in-memory key-value store.

Committer Checklist (excluded from commit message)

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

ableegoldman · 2019-02-20T00:33:59Z

@guozhangwang @vvcephei @mjsax @bbejeck

ableegoldman · 2019-02-20T00:40:13Z

I felt it was reasonable here to simply wrap a plain old Map with the required functionality, rather than take advantage of the KeyValueBytesStoreWrapper used elsewhere. This was largely to avoid having to write serdes for LRUCacheEntry, but if anyone feels strongly about it this test can be reworked to follow the others.

I think it's perfectly reasonable (and preferable) to use a simple map store so that we can more effectively isolate our testing concerns.

The only thing I'd add is that, even though it wasn't the intent of these other tests to exercise the bytes store, they did. So, we should make sure that the bytes store still has full coverage after these changes.

For this specific test though, FilteredCacheIterator is only for the caching layer not for the underlying (in-memory) store layer.

ableegoldman · 2019-02-20T00:41:00Z

I felt this was useful enough to pull out into a separate class, but perhaps this isn't the right place for it?

It seems fine to me, although a more descriptive name wouldn't hurt.

nit: we usually put such test util classes in src/test/o/a/k/test and packaged as o.a.k.test.

ableegoldman · 2019-02-20T00:42:43Z

Serialization isn't done in the most efficient way, but within the scope of this test is quite sufficient.

vvcephei

Thanks for the PR!

The change LGTM. The only concern I had was about the testing code. I'm wondering if we can eschew the new serialization wrapper store and just use your simpler KeyValueStoreWrapper implementation in the tests.

vvcephei · 2019-02-20T16:22:29Z

Would it be possible to re-write the test not to need the set serde?

IIUC, the objective is just to make sure the reduce processor invokes add/subtract properly. Previously, it was convenient to use a Set to define this logic, but it's not so convenient anymore.

I don't have a problem with your implementation, it just seems like a huge chunk of code to do something we don't strictly need to do.

Alternatively, I just saw your KeyValueStoreWrapper below. You could make that store implementation available for all the tests that don't really need to test the bytes store logic. I think I'd prefer this approach.

I'd agree, adding a StringSetSerde seems like an overkill here to me. I'd suggest we just use String as value, and reduce function like v1+v2 (there are already some mock reducers in o.a.k.test) to separate the two values for easier validation.

If we extend the KeyValueStoreWrapper idea and extract it for use by all the tests we basically end up with a copy of the old generic InMemoryKeyValueStore, that we use just for testing. Maybe that is actually the cleanest way to do things given how different tests were relying on it for different things..?

Personally, I think it is cleaner, since tests like this one get to focus more on the logic under test (the processor) and less on managing the state store layers just right, which should be the responsibility of other tests.

I also agree on the StringSetSerde is a little heavyweight.

vvcephei · 2019-02-20T16:27:11Z

I think it's perfectly reasonable (and preferable) to use a simple map store so that we can more effectively isolate our testing concerns.

The only thing I'd add is that, even though it wasn't the intent of these other tests to exercise the bytes store, they did. So, we should make sure that the bytes store still has full coverage after these changes.

vvcephei · 2019-02-20T16:28:15Z

I think I'd recommend renaming this to something more accurate (i.e., it's not a store wrapper), and elevating it to its own class so it's available for other tests.

Do we need it for other tests? Seems to be very specific. I would rather make it private.

👍 Either way. In another comment, I pointed out that other tests could be simplified by using this class instead of implementing a special serde and using the bytes store. In that case, it would be used by other tests and would need to be in its own class.

But, yes, if it stays in this test only, then it should be private.

vvcephei · 2019-02-20T16:29:48Z

It seems fine to me, although a more descriptive name wouldn't hurt.

guozhangwang

Left some very minor comments. Otherwise LGTM!

guozhangwang · 2019-02-20T17:32:08Z

nit: we usually put such test util classes in src/test/o/a/k/test and packaged as o.a.k.test.

guozhangwang · 2019-02-20T17:33:51Z

I'd agree, adding a StringSetSerde seems like an overkill here to me. I'd suggest we just use String as value, and reduce function like v1+v2 (there are already some mock reducers in o.a.k.test) to separate the two values for easier validation.

guozhangwang · 2019-02-20T17:40:53Z

For this specific test though, FilteredCacheIterator is only for the caching layer not for the underlying (in-memory) store layer.

mjsax

Thanks for the PR. Overall LGTM. Just some minor comments.

mjsax · 2019-02-20T17:45:02Z

nit: fix indention

mjsax · 2019-02-20T17:48:51Z

nit: avoid empty lines

mjsax · 2019-02-20T17:53:28Z

Do we need it for other tests? Seems to be very specific. I would rather make it private.

mjsax · 2019-02-20T17:54:24Z

nit: break too long line into one parameter per line

mjsax · 2019-02-20T17:55:54Z

nit: we should call this wrapped (to unify our naming in the code base).

Fair enough

mjsax · 2019-02-20T18:02:01Z

we should call stringSerde.close() and intSerde.close() here (in case we don't remove this code anyway)

mjsax · 2019-02-20T18:02:43Z

mjsax · 2019-02-20T18:03:06Z

mjsax · 2019-02-20T18:03:32Z

nit: use { } (preferred code style for all blocks.

mjsax · 2019-02-20T18:04:07Z

We should call stringSerde.configure(...) and intSerde.configure(...) here (in case we don't remove this code anyway)

bbejeck · 2019-02-21T23:23:28Z

retest this please

bbejeck

Sorry for the late review, just one minor comment otherwise LGTM.

bbejeck · 2019-02-21T23:32:20Z

I also agree on the StringSetSerde is a little heavyweight.

bbejeck · 2019-02-22T00:04:05Z

Failures are related. Since the MockProcessorContextTest is in test-utils package it doesn't have visibility into streams test packages. Maybe we need to move it back to the original package you had it in? (MockProcessorContext itself is not in the test package I imagine for the same reason)

…ed any tests relying on using generic parameters

guozhangwang

The current PR lgtm.

Regarding @bbejeck 's final comment about the packaging, I had a discussion with @ableegoldman and we think that a better way to keep code clean and also have small future updates is to modify the shouldStoreAndReturnStateStores test itself tot use a bytes inmemory store. And after we have KAFKA-6460 that we will have in the public APIs for those mock stores, we can then remove this class (the test-util mocks should be quite similar to this one) and let all its callers access that one.

…InMemoryKeyValueStore from a storeBuilder

guozhangwang · 2019-02-26T00:42:57Z

retest this please

guozhangwang · 2019-02-26T03:59:23Z

Merged to trunk, thanks @ableegoldman !

* AK/trunk: (36 commits) KAFKA-7962: Avoid NPE for StickyAssignor (apache#6308) Address flakiness of CustomQuotaCallbackTest#testCustomQuotaCallback (apache#6330) KAFKA-7918: Inline generic parameters Pt. II: RocksDB Bytes Store and Memory LRU Caches (apache#6327) MINOR: fix parameter naming (apache#6316) KAFKA-7956 In ShutdownableThread, immediately complete the shutdown if the thread has not been started (apache#6218) MINOR: Refactor replica log dir fetching for improved logging (apache#6313) [TRIVIAL] Remove unused StreamsGraphNode#repartitionRequired (apache#6227) MINOR: Increase produce timeout to 120 seconds (apache#6326) KAFKA-7918: Inline generic parameters Pt. I: in-memory key-value store (apache#6293) MINOR: Fix line break issue in upgrade notes (apache#6320) KAFKA-7972: Use automatic RPC generation in SaslHandshake MINOR: Enable capture of full stack trace in StreamTask#process (apache#6310) KAFKA-7938: Fix test flakiness in DeleteConsumerGroupsTest (apache#6312) KAFKA-7937: Fix Flaky Test ResetConsumerGroupOffsetTest.testResetOffsetsNotExistingGroup (apache#6311) MINOR: Update docs to say 2.2 (apache#6315) KAFKA-7672 : force write checkpoint during StreamTask #suspend (apache#6115) KAFKA-7961; Ignore assignment for un-subscribed partitions (apache#6304) KAFKA-7672: Restoring tasks need to be closed upon task suspension (apache#6113) KAFKA-7864; validate partitions are 0-based (apache#6246) KAFKA-7492 : Updated javadocs for aggregate and reduce methods returning null behavior. (apache#6285) ...

apache#6293) First PR in series to inline the generic parameters of the following bytes stores: [x] InMemoryKeyValueStore [ ] RocksDBWindowStore [ ] RocksDBSessionStore [ ] MemoryLRUCache [ ] MemoryNavigableLRUCache [ ] (awaiting merge) InMemoryWindowStore A number of tests took advantage of the generic InMemoryKeyValueStore and had to be reworked somewhat -- this PR covers everything related to the in-memory key-value store. Reviewers: Guozhang Wang <wangguoz@gmail.com>, Matthias J. Sax <mjsax@apache.org>, John Roesler <john@confluent.io>, Bill Bejeck <bbejeck@gmail.com>

ableegoldman commented Feb 20, 2019

View reviewed changes

ableegoldman changed the title ~~Inlined generic parameters Pt 1: in-memory key-value store~~ KAFKA-7918: Inline generic parameters Pt. 1: in-memory key-value store Feb 20, 2019

vvcephei reviewed Feb 20, 2019

View reviewed changes

guozhangwang approved these changes Feb 20, 2019

View reviewed changes

mjsax reviewed Feb 20, 2019

View reviewed changes

bbejeck approved these changes Feb 21, 2019

View reviewed changes

bbejeck added the streams label Feb 22, 2019

ableegoldman added 3 commits February 25, 2019 12:31

Inlined generic parameters for in-memory key-value byte store and fix…

c39ae20

…ed any tests relying on using generic parameters

Minor fixes and cosmetic changes from PR review

2efda8f

Added GenericInMemoryKeyValueStore to clean up testing

ea571b9

ableegoldman force-pushed the InlineByteStoreTypes branch from 2574960 to ea571b9 Compare February 25, 2019 20:36

guozhangwang reviewed Feb 25, 2019

View reviewed changes

MockProcessorContextTest: replaced GenericInMemoryKeyValueStore with …

fc28525

…InMemoryKeyValueStore from a storeBuilder

ableegoldman changed the title ~~KAFKA-7918: Inline generic parameters Pt. 1: in-memory key-value store~~ KAFKA-7918: Inline generic parameters Pt. I: in-memory key-value store Feb 26, 2019

guozhangwang merged commit 21b7963 into apache:trunk Feb 26, 2019

Conversation

ableegoldman commented Feb 20, 2019

Committer Checklist (excluded from commit message)

Uh oh!

ableegoldman commented Feb 20, 2019

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!

vvcephei 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!

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!

guozhangwang 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!

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!

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 commented Feb 21, 2019