CAMEL-24595: CassandraKeyValueRepository: implement atomic replace() and delete(key, expected) using LWT - #26053
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
✅ Generated files are up to dateAn earlier CI run reported uncommitted generated changes; the latest run no longer does. |
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 9 tested, 27 compile-only — current: 9 all testedMaveniverse Scalpel detected 36 affected modules (current approach: 9).
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for closing the gap on this, Guillaume — nice, tight change. It follows the existing patterns closely (init*Statement() in doStart(), applyConsistencyLevel(..., writeConsistencyLevel), isApplied(), generateDelete(...) reuse), and I verified the LWT bind order against the generated CQL text for all three statements — UPDATE ... USING TTL ? SET value = ? WHERE key = ? IF value = ? → bind(ttl, newValue, key, expected), and likewise for the non-TTL and delete variants. The missing-key → not-applied → false behavior matches the javadoc for both operations. Good stuff.
Two minor, non-blocking notes:
1. Comparison is by serialized bytes, not Objects.equals (see inline). The SPI default replace/delete compare the deserialized current value with Objects.equals(current, expected), whereas the LWT IF value = ? compares the serialized ByteBuffer. That's equivalent only when serialization is deterministic and the caller passes the exact stored value; two .equals()-equal objects that serialize differently would diverge from the default impl. This is a reasonable (arguably stricter) approach for server-side CAS — just worth a short Javadoc note so the contract is explicit.
2. Test gap for replace(...) with a TTL. All existing testReplace* ITs pass ttl = null, so the new updateIfValueWithTtlStatement branch (ttlSeconds > 0) and its distinct bind order aren't exercised. A testReplaceWithTtl IT would guard that path. The PR body's claim that the existing ITs "already cover both operations" holds for the no-TTL paths but not the TTL branch.
Neither is a blocker. This is a rules-and-conventions and diff review; it does not replace specialized tools such as CodeRabbit/Sourcery or SonarCloud static analysis.
Claude Code on behalf of davsclaus
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| public boolean replace(String key, Object expectedOldValue, Object newValue, Duration ttl) { | ||
| LOGGER.debug("Replacing key {} if value matches, TTL {}", key, ttl); | ||
| ByteBuffer serializedNewValue = KeyValueRepositoryHelper.serializeToByteBuffer(newValue); | ||
| ByteBuffer serializedExpectedValue = KeyValueRepositoryHelper.serializeToByteBuffer(expectedOldValue); |
There was a problem hiding this comment.
The LWT IF value = ? compares the serialized ByteBuffer of expectedOldValue against the stored bytes, whereas the KeyValueRepository SPI default compares deserialized values with Objects.equals(current, expectedOldValue). These match only if serialization is deterministic and the caller passes the exact value that was stored.
Edge case: two objects that are .equals() but serialize to different bytes (e.g. maps/sets with different iteration order, or different concrete collection types) would return false here where the default replace/delete returns true. This is a reasonable and arguably more correct choice for server-side CAS — a short Javadoc note stating the comparison is by serialized value (same for delete(key, expected)) would make the contract explicit for callers.
…and delete(key, expected) using lightweight transactions
2 participants
What
Implement atomic
replace()anddelete(key, expected)inCassandraKeyValueRepositoryusing Cassandra lightweight transactions (LWT), as flagged by @davsclaus in #25993.Why
putIfAbsent()already uses LWT (INSERT ... IF NOT EXISTS) for true server-side CAS, butreplace()anddelete(key, expected)fell back to the non-atomic default implementation (read-then-write) from theKeyValueRepositorySPI.How
replace(key, expected, newValue, ttl): UsesUPDATE ... SET value = ? WHERE key = ? IF value = ?(with optionalUSING TTL ?)delete(key, expected): UsesDELETE FROM ... WHERE key = ? IF value = ?Both methods use the existing
isApplied()helper to check the LWT result. Three new prepared statements are initialized at startup (updateIfValueStatement,updateIfValueWithTtlStatement,deleteIfValueStatement).The existing integration tests in
CassandraKeyValueRepositoryITalready cover both operations (testReplaceMatchingOldValue, testReplaceNonMatchingOldValue, testReplaceMissingKey, testDeleteWithExpectedValue*).