jerrypeng commented on PR #59953: URL: https://github.com/apache/spark/pull/59953#issuecomment-6535106821
@shenavaa Thanks for calling this out. I agree the re-read is necessary with logical-expiry semantics: an expired-but-physically-present key can be refreshed before the RocksDB snapshot iterator reaches it, so deleting based solely on the snapshot value could remove the replacement. In #59762’s current implementation, we check store.keyExists(key) before cleanup and insert only if that check returns false. An expired entry that is still physically present therefore suppresses the incoming record rather than being refreshed. A replacement can be inserted only after cleanup has removed the old entry, at which point its snapshot entry has already been consumed. That prevents this particular stale-snapshot deletion scenario without a re-read. If we adopt the logical-expiry check, I agree that current-value revalidation and your regression test should come with it. Independently, an ordered-key RocksDB regression covering removal followed by same-batch reinsertion would be useful coverage for the current approach too. Thanks for adding the concrete test—it helps clarify the invariant each approach relies on. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
