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

2023-02-02 Thread via GitHub


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

   `TEST_CASES` is clear and also the names of the test methods are clear. It's 
really the code iterating over the test cases with all the loops (forward, 
backward) and nested if/else control flow. I understand _why_ you wrote it this 
way, but reviewing/reading it is very hard, and so I have personally not a very 
high confidence that I understand if the test is really verifying what it is 
supposed to test?
   
   I agree that it's important to test all those combinations w/ and w/o 
tombstones etc,
   
   Bottom line: I don't have a good suggestion... The only thing could be to 
write each case individually, but it's a lot of redundant code and very verbose 
so also not really great...


-- 
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



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

2023-02-01 Thread via GitHub


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

   Most of my comments are educational or nits. I merged this PR already, and 
if you want to address anything (feel free to ignore), let's do a follow up PR. 
Wanted to merge this to unblock other work that is lined up.


-- 
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