Fix HINCRBYFLOAT removes field expiration on replica#22
Conversation
WalkthroughThe changes introduce HSETEX as a new shared command object and modify HINCRBYFLOAT's replication behavior to preserve field expiration times using HSETEX with KEEPTTL instead of HSET with KEEP_TTL flag, with corresponding test coverage for replica synchronization. Changes
Sequence DiagramsequenceDiagram
participant Master
participant Replica
participant ReplicationStream
Master->>Master: HINCRBYFLOAT executed<br/>(field has TTL)
Master->>Master: Update field in-memory<br/>(no KEEP_TTL flag)
Master->>ReplicationStream: Emit HSETEX cmd<br/>(preserves TTL via vector)
ReplicationStream->>Replica: Transmit HSETEX with KEEPTTL
Replica->>Replica: Apply HSETEX<br/>(field retains TTL)
Note over Master,Replica: TTL consistency maintained<br/>across replication
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/t_hash.c`:
- Line 2559: The call to hashTypeSet should preserve existing field TTLs: modify
the invocation of hashTypeSet in the HINCRBYFLOAT (the line calling
hashTypeSet(c->db, o, c->argv[2]->ptr, new, HASH_SET_TAKE_VALUE)) to include the
HASH_SET_KEEP_TTL flag (i.e., pass HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL) so
masters do not drop TTLs and remain consistent with replicas; update the single
call site referencing hashTypeSet in the HINCRBYFLOAT handling code accordingly.
- Around line 2571-2572: The call to rewriteClientCommandVector currently uses
argc=6 but passes seven robj* arguments for the HSETEX command, causing the
final newobj (value) to be omitted; update the argc parameter to 7 in the
rewriteClientCommandVector invocation so the args (shared.hsetex, c->argv[1],
shared.keepttl, shared.fields, shared.integers[1], c->argv[2], newobj) are all
included, and keep the argument order as shown to match the expected HSETEX
<key> KEEPTTL FIELDS 1 <field> <value> replication format.
🧹 Nitpick comments (1)
tests/unit/type/hash-field-expire.tcl (1)
1960-2004: Test verifies replica behavior but should also verify master TTL preservation.The test correctly validates:
- Replication stream contains
HSETEXwithKEEPTTL- TTL is preserved on replica after
HINCRBYFLOATHowever, the test doesn't explicitly verify that TTL is preserved on the master after
HINCRBYFLOAT. Given the implementation issues insrc/t_hash.c(missingHASH_SET_KEEP_TTLflag), the master would actually lose the TTL while the replica preserves it.Consider adding an assertion to verify master-side TTL preservation before replication sync:
♻️ Suggested improvement
r -1 hincrbyfloat h1 f1 1.1 r -1 hincrbyfloat h1 f2 1.1 + # Verify TTL is preserved on master after HINCRBYFLOAT + assert_range [r -1 httl h1 FIELDS 1 f1] 90 100 + # Expiration time should not be removed on replica and the value # should be equal to the master. wait_for_ofs_sync [srv -1 client] [srv 0 client]
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/server.csrc/server.hsrc/t_hash.ctests/unit/type/hash-field-expire.tcl
🧰 Additional context used
🧬 Code graph analysis (1)
src/server.c (1)
src/object.c (1)
createStringObject(330-335)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: test-ubuntu-latest
- GitHub Check: build-centos-jemalloc
- GitHub Check: test-sanitizer-address
- GitHub Check: build-32bit
- GitHub Check: build-debian-old
- GitHub Check: build-old-chain-jemalloc
- GitHub Check: code-coverage
- GitHub Check: test-external-nodebug
- GitHub Check: test-external-cluster
- GitHub Check: test-external-standalone
- GitHub Check: Analyze (cpp)
- GitHub Check: code-coverage
- GitHub Check: test-sanitizer-address
- GitHub Check: build-32bit
- GitHub Check: build-centos-jemalloc
- GitHub Check: build-old-chain-jemalloc
- GitHub Check: build-debian-old
- GitHub Check: test-ubuntu-latest
- GitHub Check: test-external-cluster
- GitHub Check: test-external-standalone
- GitHub Check: test-external-nodebug
🔇 Additional comments (5)
src/server.h (1)
1529-1531: LGTM!The addition of
*hsetexand*fieldsshared objects tosharedObjectsStructis correctly placed and aligns with the PR objective. These shared objects will be used to propagate HINCRBYFLOAT via HSETEX with KEEPTTL to preserve field expiration times on replicas.src/server.c (1)
2130-2153: LGTM — shared command/arg constants look consistent with existing patterns.src/t_hash.c (2)
2565-2568: Comment is accurate but implementation is inconsistent.The comment correctly states that
KEEPTTLflag preserves field TTL. However, this only applies to the replicated command. The localhashTypeSetcall (line 2559) currently does NOT preserve TTL on the master due to the missingHASH_SET_KEEP_TTLflag.
2576-2576: LGTM - Whitespace/formatting change only; function signature remains unchanged.tests/unit/type/hash-field-expire.tcl (1)
1970-1976: Test expectations are correct and will catch the argc bug if present.The expected replication stream patterns like
{hsetex h1 KEEPTTL FIELDS 1 f1 *}require 7 tokens. If theargc=6bug causes the value to be omitted, the pattern matching should fail, making this a good regression test.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| int len = ld2string(buf,sizeof(buf),value,LD_STR_HUMAN); | ||
| new = sdsnewlen(buf,len); | ||
| hashTypeSet(c->db, o,c->argv[2]->ptr,new,HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL); | ||
| hashTypeSet(c->db, o,c->argv[2]->ptr,new,HASH_SET_TAKE_VALUE); |
There was a problem hiding this comment.
Critical: Missing HASH_SET_KEEP_TTL causes TTL removal on master.
The HASH_SET_KEEP_TTL flag has been removed from the hashTypeSet call. Looking at the hashTypeSet implementation (lines 959-964 and 988-991), when this flag is NOT set and the field has a TTL, the TTL is removed.
This creates master/replica inconsistency:
- Master: TTL is removed (no
HASH_SET_KEEP_TTL) - Replica: TTL is preserved (receives
HSETEX ... KEEPTTL)
Additionally, the existing test at lines 851-857 expects TTL to be preserved on master:
test {HINCRBYFLOAT - preserve expiration time of the field} {
...
assert_range [r HPTTL h1 FIELDS 1 f1] 1 20
}Compare with HINCRBY at line 2506 which correctly uses HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL.
🐛 Proposed fix
- hashTypeSet(c->db, o,c->argv[2]->ptr,new,HASH_SET_TAKE_VALUE);
+ hashTypeSet(c->db, o,c->argv[2]->ptr,new,HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL);🤖 Prompt for AI Agents
In `@src/t_hash.c` at line 2559, The call to hashTypeSet should preserve existing
field TTLs: modify the invocation of hashTypeSet in the HINCRBYFLOAT (the line
calling hashTypeSet(c->db, o, c->argv[2]->ptr, new, HASH_SET_TAKE_VALUE)) to
include the HASH_SET_KEEP_TTL flag (i.e., pass HASH_SET_TAKE_VALUE |
HASH_SET_KEEP_TTL) so masters do not drop TTLs and remain consistent with
replicas; update the single call site referencing hashTypeSet in the
HINCRBYFLOAT handling code accordingly.
| rewriteClientCommandVector(c, 6, shared.hsetex, c->argv[1], shared.keepttl, | ||
| shared.fields, shared.integers[1], c->argv[2], newobj); |
There was a problem hiding this comment.
Critical: Wrong argc value causes value to be missing from replicated command.
The rewriteClientCommandVector call specifies argc=6, but 7 robj* arguments are passed:
shared.hsetex- "HSETEX"c->argv[1]- keyshared.keepttl- "KEEPTTL"shared.fields- "FIELDS"shared.integers[1]- "1"c->argv[2]- fieldnewobj- value ← This will be excluded!
The expected HSETEX command format requires 7 arguments: HSETEX <key> KEEPTTL FIELDS 1 <field> <value>
With argc=6, the replicated command will be missing the value, causing data corruption on replicas.
🐛 Proposed fix
- rewriteClientCommandVector(c, 6, shared.hsetex, c->argv[1], shared.keepttl,
+ rewriteClientCommandVector(c, 7, shared.hsetex, c->argv[1], shared.keepttl,
shared.fields, shared.integers[1], c->argv[2], newobj);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rewriteClientCommandVector(c, 6, shared.hsetex, c->argv[1], shared.keepttl, | |
| shared.fields, shared.integers[1], c->argv[2], newobj); | |
| rewriteClientCommandVector(c, 7, shared.hsetex, c->argv[1], shared.keepttl, | |
| shared.fields, shared.integers[1], c->argv[2], newobj); |
🤖 Prompt for AI Agents
In `@src/t_hash.c` around lines 2571 - 2572, The call to
rewriteClientCommandVector currently uses argc=6 but passes seven robj*
arguments for the HSETEX command, causing the final newobj (value) to be
omitted; update the argc parameter to 7 in the rewriteClientCommandVector
invocation so the args (shared.hsetex, c->argv[1], shared.keepttl,
shared.fields, shared.integers[1], c->argv[2], newobj) are all included, and
keep the argument order as shown to match the expected HSETEX <key> KEEPTTL
FIELDS 1 <field> <value> replication format.
2 participants
Benchmark PR from qodo-benchmark#106
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.