fix: Bypass window store cache when doing windowed pull queries #6548
Conversation
There was a problem hiding this comment.
Overall LGTM. Maybe we can only answer whether the performance is similar to the case with caching disabled via config directly by doing some benchmarks. I'm just wondering if calling reflection per-fetch call would be too costly or is it negligible. If benchmark shows latter case, we can consider caching the provider / type along with the CompositeReadOnlyXXStore (actually, does that work without changing streams code? anyways, we'll see if it is really necessary)
There was a problem hiding this comment.
Thanks for this, @AlanConfluent ! It looks good to me. I just had a couple of minor comments.
| if (!(store instanceof SessionStore)) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
This is interesting; why does it happen?
...Ah, I see. It's because the RocksDBSessionStore wraps a SegmentedBytesStore.
The rest seems fairly obvious, but this one might be subtle enough to warrant a comment.
There was a problem hiding this comment.
Yes, it's exactly as you mentioned. Stopping at the last SessionStore is sufficient since it is below the cache and still has the expected interface for fetching. Added a few comments about that.
| } | ||
|
|
||
| @Test | ||
| public void shouldAvoidNonWindowStore() throws IllegalAccessException { |
There was a problem hiding this comment.
It looks like this test verifies that we bottom out at a wrapped store when it doesn't wrap a WindowStore, right?
It seems like a similar test could positively verify that we actually do skip the caching layer, or any other wrapped layer, but I didn't see that test. Did I miss it?
There was a problem hiding this comment.
The logic was technically cache agnostic. So long as the last WindowStore layer is not a cache, which we know it wouldn't be since the caching layer is built upon that, this check would hopefully be sufficient.
I changed the bypassing logic so that it verifies that it actually passes the caching layer when it's run and the tests verify that as well.
There was a problem hiding this comment.
Actually, I removed the bypass check because it's possible that the user can configure it without a cache. I don't really want to try to reproduce the logic for determining if cache is enabled, so I removed this bypass check.
AlanConfluent
force-pushed
the
remove_window_store_cache
branch
from
0aa7346
to
7cfe7de
Compare
From my benchmarking, the reflection doesn't appear to have a significant effect on performance. I think the performance is dominated by other factors, namely query parsing, and actually doing the reads from rocksdb. I'm hitting more or less the same numbers as with caching disabled. On our standard single node windowed benchmark, we can do about 1800qps, and in benchmarks with this change, I'm seeing 1850 qps on my latest run, so the overhead is well within the noise. |
That's great!! |
| @@ -129,6 +129,7 @@ | |||
| .withProperty(KsqlRestConfig.ADVERTISED_LISTENER_CONFIG, "http://localhost:8188") | |||
| .withProperty(KsqlConfig.KSQL_QUERY_PULL_ENABLE_STANDBY_READS, true) | |||
| .withProperty(KsqlConfig.KSQL_STREAMS_PREFIX + "num.standby.replicas", 1) | |||
| .withProperty(KsqlConfig.KSQL_STREAMS_PREFIX + "cache.max.bytes.buffering", 10000) | |||
There was a problem hiding this comment.
I just added them to one of the pull query tests because I wanted to exercise the cache bypassing code in a functional test. It's hard to set up a realistic scenario in unit tests since it's not easy to construct all of the state store layers as they actually exist.
If something basic goes wrong in the reflection or types with underlying streams when faced with the cache, these tests should fail. The real test for this will be the automated benchmarks which test that this bypassing effect works as intended.
AlanConfluent
changed the title
Description
Windowed pull queries have very mediocre performance. After lots of investigation, it was clear that this was due to the use of the streams cache. We experimented with disabling the cache, and performance is good for pull queries. The issue is that if we did this, then other areas such as persistent queries suffer in their performance.
This PR aims to disable the use of the streams cache for only windowed pull queries. There was a lot of discussion over whether a public API should be exposed in Streams to bypass the cache during a state store lookup. At the moment they don't want to do this with the existing API. Next public API will expose bypassing the cache.
In order to give ksqlDB good performance for all pull query types, it was decided to use some reflection to bypass the lack of proper public APIs and skip the caching layers.
Testing done
Ran local unit tests. Ran
RestQueryTranslationTests. Ran pull query benchmarks for windowed queries.Reviewer checklist