RocMarshal edited a comment on pull request #17692: URL: https://github.com/apache/flink/pull/17692#issuecomment-969035044
> The situation is a bit odd. I think we shouldn't have a cache on top of rocks DB at all. We could think about having a cache in rocks DB itself. > > Nevertheless, the cache is there and causes trouble for you. I understood now that your changes are not impacting correctness but only performance, so I think your contribution certainly is welcome. > > To reiterate from my first rough review: Could you please add a (unit) test and make the configuration descriptions easier to understand for folks that have not read the CEP code? Thanks so much for your @AHeise reply and explanation, which makes me suddenly see the light. As updated in the `CEPCacheOptions`: ` And it could accelerate the CEP operate process speed and limit the capacity of cache in pure memory. Note: It's only effective to limit usage of memory when 'state.backend' was set as 'rocksdb', which would transport the elements exceeded the number of the cache into the rocksdb state storage instead of memory state storage. ` The minor pr is really helpful for memory limitation when the `state.backend` is set as `rocksdb`. By contrast，when the `state.backend` is set as not `rocksdb`, the current cache in the pr would cause performance decreased. Compared with old cache , the state part will contain more elements swapped out from new guava-cache ,which would make it heavier to `copy on write` for state . In my limited read, if the positive significance of the pr is bigger than the side-effects of the pr, it would be worthy to do. And Please let me know what do you think of it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
