KAFKA-13785: [10/N][emit final] more unit test for session store and disable cache for emit final sliding window - #12370
Conversation
There was a problem hiding this comment.
Why do we need this call? I thought we only add tests in this PR? Is the feature not completed yet and we need this?
There was a problem hiding this comment.
Want to make it explicit in case default is caching enabled. I can remove it if you think it's unnecessary.
There was a problem hiding this comment.
Maybe @guozhangwang can chime in?
I am happy both ways I guess, but we should keep it consistent if possible.
There was a problem hiding this comment.
We would only default at the Materialized level, so the if condition should be sufficient even if it's default to turn on caching, but nevertheless making it explicit seems fine.
There was a problem hiding this comment.
We would only default at the Materialized level, so the if condition should be sufficient even if it's default to turn on caching, but nevertheless making it explicit seems fine.
There was a problem hiding this comment.
Just wondering, since we are leveraging on the inherited abstract session bytes store test, are there any test cases below that can be moved there or even are duplicates?
There was a problem hiding this comment.
For shouldRemoveExpired, the RocksDB version has different params for session window.
For shouldNotExpireFromOpenIterator, it's not supported in RocksDBSessionStore
For shouldMatchPositionAfterPut, the RocksDB version has different cast than InMemorySessionStore.
So we could put shouldRemoveExpired and shouldMatchPositionAfterPut in parent class and add different logic depending on store type. We can also leave it here. wdyt?
There was a problem hiding this comment.
This is for line 52 above: should we rename that function to a more meaningful one? maybe getParamStoreType?
There was a problem hiding this comment.
Could we also add a "miss" case for fetchSession here?
|
@lihaosky Thanks for your PR, I made a pass on it. |
54d2cbe to
0229a14
Compare
|
LGTM! I'm merging to trunk now. |
…disable cache for emit final sliding window (apache#12370) 1. Added more unit test for RocksDBTimeOrderedSessionStore and RocksDBTimeOrderedSessionSegmentedBytesStore 2. Disable cache for sliding window if emit strategy is ON_WINDOW_CLOSE Reviewers: Matthias J. Sax <matthias@confluent.io>, Guozhang Wang <wangguoz@gmail.com>
3 participants
RocksDBTimeOrderedSessionStoreandRocksDBTimeOrderedSessionSegmentedBytesStoreON_WINDOW_CLOSECommitter Checklist (excluded from commit message)