[GitHub] [kafka] vcrfxia commented on pull request #13126: KAFKA-14491: [1/N] Add segment value format for RocksDB versioned store

["vcrfxia (via GitHub)" <gi...@apache.org>]

vcrfxia commented on PR #13126:
URL: https://github.com/apache/kafka/pull/13126#issuecomment-1413015517

   Thanks, @mjsax ! Addressed your latest review comments in https://github.com/apache/kafka/pull/13186 and responded inline here.
   
   > The test cases are really hard to read IMHO, because they are super complex. Not sure if we should make this simpler?
   
   Do you mean the cases themselves (in the `TEST_CASES` list), or the unit tests (e.g., `shouldSerializeAndDeserialize()`, `shouldInsertAtIndex()`, etc)?
   
   For the former, I tried to give them helpful names since I know it's a lot to read. Do you think it would help to change the format that they're defined in? Or if you have other suggestions I'm all ears too. I do think it's important to test combinations with adjacent tombstones, a mix of tombstones and non-tombstones, etc, though, in order to hit all the different code paths in the class itself.
   
   For the latter, the test logic for some of the unit tests is long but each test is very targeted (tests a specific method in the class). I could make this easier to read by extracting test setup into helper methods. Let me know if you think that would help.


-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org