PragmaTwice commented on PR #3269: URL: https://github.com/apache/kvrocks/pull/3269#issuecomment-3573695631
Hi, thank you for your contribution. I skimmed through and found some issue in this design: - "New format (with expiration): [0xFF][flags][8-byte timestamp][value]". Hmm the hash field value can actually be binary, so even in the old encoding, the first byte of the value CAN be `0xff`. How to distinguish them? I think the answer is negative. - `-2 = field doesn't exist, 1 = expiration set, 0 = expiration not set (e.g., invalid expire time)`. Usually this comment indicates that you should use enum classes instead of integers. - the `size` field in metatdata seems not well mantained, in compaction filters. - it seems `HashFieldValue` can be a view (zero-copy)? - The code seems vibe-coded. It is fine but there are lots of useless code comment and the PR author should understand details of the llm-written code (so that the review process can be meaningful). -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
