Fix HINCRBYFLOAT removes field expiration on replica#31
Conversation
WalkthroughThe changes introduce support for HSETEX command in shared objects and modify HINCRBYFLOAT to replicate with explicit TTL preservation using HSETEX with KEEPTTL flag instead of HSET, along with comprehensive test validation for replica TTL semantics. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Master as Master Server
participant Replica as Replica Server
Client->>Master: HINCRBYFLOAT key field increment
activate Master
Master->>Master: hashTypeSet (without KEEP_TTL flag)
Master->>Master: rewriteClientCommandVector<br/>(construct HSETEX with KEEPTTL)
deactivate Master
Master->>Replica: HSETEX key field value KEEPTTL
activate Replica
Replica->>Replica: Process HSETEX<br/>(preserve existing TTL)
deactivate Replica
Client->>Replica: Verify field TTL
Replica-->>Client: TTL preserved ✓
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`:
- Around line 2565-2572: The rewriteClientCommandVector call for HINCRBYFLOAT
builds 7 arguments but passes argc=6, which drops the value; update the call to
pass the correct argc (7) so the rewritten HSETEX includes the new value object
(newobj). Locate the rewriteClientCommandVector invocation in t_hash.c (the call
using shared.hsetex, shared.keepttl, shared.fields, shared.integers[1],
c->argv[2], newobj) and change its argc parameter to 7 to match the supplied
arguments.
- Around line 2556-2560: The call to hashTypeSet in the HINCRBYFLOAT update
drops the HASH_SET_KEEP_TTL flag causing field TTLs on the primary to be lost;
restore preservation of TTL by passing HASH_SET_KEEP_TTL along with
HASH_SET_TAKE_VALUE to hashTypeSet (i.e., call hashTypeSet(c->db, o,
c->argv[2]->ptr, new, HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL)) so the existing
field expiration is kept consistent with replicas and the documented
HINCRBYFLOAT behavior.
📜 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 (2)
src/server.c (1)
src/object.c (1)
createStringObject(330-335)
src/t_hash.c (2)
src/networking.c (1)
rewriteClientCommandVector(4158-4174)src/object.c (1)
createRawStringObject(131-133)
⏰ 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: build-centos-jemalloc
- GitHub Check: test-ubuntu-latest
- GitHub Check: build-debian-old
- GitHub Check: build-old-chain-jemalloc
- GitHub Check: test-sanitizer-address
- GitHub Check: build-32bit
- GitHub Check: test-external-cluster
- GitHub Check: Analyze (cpp)
- GitHub Check: test-external-nodebug
- GitHub Check: test-external-standalone
- GitHub Check: code-coverage
- GitHub Check: build-old-chain-jemalloc
- GitHub Check: code-coverage
- GitHub Check: test-external-cluster
- GitHub Check: test-external-nodebug
- GitHub Check: test-external-standalone
- GitHub Check: build-centos-jemalloc
- GitHub Check: build-debian-old
- GitHub Check: test-ubuntu-latest
- GitHub Check: build-32bit
- GitHub Check: test-sanitizer-address
🔇 Additional comments (5)
src/server.c (2)
2130-2131: Looks good: shared HSETEX command initialized consistently.Matches the existing shared command name initialization pattern in this block.
2152-2154: Looks good: shared FIELDS token added in the right section.Consistent with other shared command-argument tokens in this area.
src/server.h (1)
1529-1531: LGTM!The new shared object declarations for
hsetexandfieldsfollow the established pattern insharedObjectsStruct. These are correctly placed among related hash command objects and keyword objects respectively, and will be used to support TTL-preserving replication forHINCRBYFLOATviaHSETEXwithKEEPTTL.src/t_hash.c (1)
2576-2597: No issues to flag in this helper.tests/unit/type/hash-field-expire.tcl (1)
1960-2004: Good coverage for replica TTL preservation and replication stream.
This test clearly validates the new HSETEX+KEEPTTL replication path and checks replica state consistency.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| char buf[MAX_LONG_DOUBLE_CHARS]; | ||
| 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); | ||
| addReplyBulkCBuffer(c,buf,len); |
There was a problem hiding this comment.
Preserve TTL on primary when updating HINCRBYFLOAT.
Line 2559 drops HASH_SET_KEEP_TTL, which removes existing field TTLs on the primary and can diverge from replicas now that KEEPTTL is used in replication. This also contradicts the existing “HINCRBYFLOAT - preserve expiration time of the field” behavior.
🔧 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` around lines 2556 - 2560, The call to hashTypeSet in the
HINCRBYFLOAT update drops the HASH_SET_KEEP_TTL flag causing field TTLs on the
primary to be lost; restore preservation of TTL by passing HASH_SET_KEEP_TTL
along with HASH_SET_TAKE_VALUE to hashTypeSet (i.e., call hashTypeSet(c->db, o,
c->argv[2]->ptr, new, HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL)) so the existing
field expiration is kept consistent with replicas and the documented
HINCRBYFLOAT behavior.
| /* Always replicate HINCRBYFLOAT as an HSETEX command with the final value | ||
| * in order to make sure that differences in float precision or formatting | ||
| * will not create differences in replicas or after an AOF restart. */ | ||
| * will not create differences in replicas or after an AOF restart. | ||
| * The KEEPTTL flag is used to make sure the field TTL is preserved. */ | ||
| robj *newobj; | ||
| newobj = createRawStringObject(buf,len); | ||
| rewriteClientCommandArgument(c,0,shared.hset); | ||
| rewriteClientCommandArgument(c,3,newobj); | ||
| 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.
Fix argc mismatch in rewriteClientCommandVector.
Line 2571 passes 7 arguments but sets argc to 6, which drops the value argument and rewrites an invalid HSETEX command.
🔧 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.
| /* Always replicate HINCRBYFLOAT as an HSETEX command with the final value | |
| * in order to make sure that differences in float precision or formatting | |
| * will not create differences in replicas or after an AOF restart. */ | |
| * will not create differences in replicas or after an AOF restart. | |
| * The KEEPTTL flag is used to make sure the field TTL is preserved. */ | |
| robj *newobj; | |
| newobj = createRawStringObject(buf,len); | |
| rewriteClientCommandArgument(c,0,shared.hset); | |
| rewriteClientCommandArgument(c,3,newobj); | |
| rewriteClientCommandVector(c, 6, shared.hsetex, c->argv[1], shared.keepttl, | |
| shared.fields, shared.integers[1], c->argv[2], newobj); | |
| /* Always replicate HINCRBYFLOAT as an HSETEX command with the final value | |
| * in order to make sure that differences in float precision or formatting | |
| * will not create differences in replicas or after an AOF restart. | |
| * The KEEPTTL flag is used to make sure the field TTL is preserved. */ | |
| robj *newobj; | |
| newobj = createRawStringObject(buf,len); | |
| 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 2565 - 2572, The rewriteClientCommandVector call
for HINCRBYFLOAT builds 7 arguments but passes argc=6, which drops the value;
update the call to pass the correct argc (7) so the rewritten HSETEX includes
the new value object (newobj). Locate the rewriteClientCommandVector invocation
in t_hash.c (the call using shared.hsetex, shared.keepttl, shared.fields,
shared.integers[1], c->argv[2], newobj) and change its argc parameter to 7 to
match the supplied arguments.
2 participants
Benchmark PR from agentic-review-benchmarks#9
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.