vamossagar12 commented on pull request #11211: URL: https://github.com/apache/kafka/pull/11211#issuecomment-907658860
> @vamossagar12 , thanks for the PR. Have a quick look, and left some comments. But most importantly, the test coverage is not enough. I saw you changed `RocksDBWindowStore`, `RocksDBSessionStore`, but there are no tests for them. Also, no integration tests for this change. I expected we should have a `WindowStore` test and a `SessionStore` test to see if we can actually remove the records after retention time. What do you think? > > Thank you. @showuon , thank you. The changes in `RocksDBWindowStore` and `RocksDBSessionStore` are minimal(only changing the parent class). The real filtering logic is in Metered classes and that's where I have added all the tests by mocking the above 2 classes and so I thought we won't need unit tests for `RocksDBWindowStore` and `RocksDBSessionStore`. WDYT? +1 for integration test though. Would add. -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
