KAFKA-3452 Follow-up: Refactoring StateStore hierarchies#2360
KAFKA-3452 Follow-up: Refactoring StateStore hierarchies#2360dguy wants to merge 4 commits intoapache:trunkfrom
Conversation
|
@guozhangwang, @mjsax, @enothereska - this is the refactor of the store hierarchies along the lines of the suggestions made by @guozhangwang |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
guozhangwang
left a comment
guozhangwang
left a comment
There was a problem hiding this comment.
I feel myself is not the ideal reviewer since I'm too familiar to most of the changes. I focused on the added unit tests, besides I left some discussion questions.
| import org.apache.kafka.streams.state.WindowStore; | ||
| import org.apache.kafka.streams.state.WindowStoreIterator; | ||
|
|
||
| public class MeteredWindowStore<K, V> implements WindowStore<K, V> { |
There was a problem hiding this comment.
I was a bit pondering on this function: for window and session store RocksDB impl, its wrapping hierarchy is a bit different with the key-value store, as it is
caching -> window -> metered-segmented -> changelog-segmented -> raw-segmented (rocksdb)
Where key-value store hierarchy is
caching -> metered -> changelog -> rocksdb
If users provide a customized window store implementation, I think it would be similar to key-value as
caching -> metered -> changelogged -> user-provided window-store
I think case we still need to provide the MeteredWindowStore and ChangeLoggingWindowStore wrappers. WDYT?
There was a problem hiding this comment.
If people provide their own store implementations, then we aren't wrapping them at all at the moment. They would provide a custom StateStoreSupplier and we just call that to get the store.
The question is do we want to have code that we don't use, yet? Should we just add them back if we ever provide a way to wrap the store that is created from a custom StateStoreSupplier?
There was a problem hiding this comment.
Sounds good. Let's worry about that later.
| private final String name; | ||
| private final Segments segments; | ||
| private final KeySchema keySchema; | ||
| private final boolean loggingEnabled; |
There was a problem hiding this comment.
As we discussed before, this seems not needed any more?
There was a problem hiding this comment.
Yep - i removed it already
|
|
||
| public RocksDBStore(String name, Serde<K> keySerde, Serde<V> valueSerde) { | ||
| this(name, DB_FILE_DIR, keySerde, valueSerde); | ||
| public RocksDBStore(final String name, |
There was a problem hiding this comment.
We should be able to remove logging out of RocksDBStore now, right?
There was a problem hiding this comment.
Yeah - i've done that. Though it looks like i forgot to remove the field
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
retest this please. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
LGTM and merged to trunk, piggy-backed to 0.10.2. |
This is a follow up of apache#2166 - refactoring the store hierarchies as requested Author: Damian Guy <damian.guy@gmail.com> Reviewers: Guozhang Wang <wangguoz@gmail.com> Closes apache#2360 from dguy/state-store-refactor
3 participants
This is a follow up of #2166 - refactoring the store hierarchies as requested