HFE to support AOF and replicas by moticless · Pull Request #13285 · redis/redis

moticless · 2024-05-21T12:09:09Z

For replica sake, rewrite commands H*EXPIRE* , HSETF, HGETF to have absolute unix time in msec.
On active-expiration of field, propagate HDEL to replica (propagateHashFieldDeletion())
On lazy-expiration, propagate HDEL to replica (hashTypeGetValue() now calls hashTypeDelete(). It also takes care to call propagateHashFieldDeletion()).
Fix H*EXPIRE* command such that if it gets flag LT and it doesn’t have any expiration on the field then it will considered as valid condition.

Note, replicas doesn’t make any active expiration, and should avoid lazy expiration. On hashTypeGetValue() it doesn't check expiration (As long as the master didn’t request to delete the field, it is valid)

TODO:

Attach dbid to HASH metadata. See here

sundb · 2024-05-23T05:08:54Z

@moticless should the TODO in https://github.com/redis/redis/blob/a25b15392c3f50909ab2dafbcbdb3794ada29620/src/rdb.c#L2312:L2315 be handled here?

moticless · 2024-05-23T11:54:15Z

@moticless should the TODO in https://github.com/redis/redis/blob/a25b15392c3f50909ab2dafbcbdb3794ada29620/src/rdb.c#L2312:L2315 be handled here?

I will handle it on the next commit.

tezc · 2024-05-27T14:40:46Z

-    /* Delete all the expired fields in one go */
-    if (r.expired > 0)
-        lpt->lp = lpDeleteRange(lpt->lp, 0, r.expired * 3);
+        ptr = lpNext(lpt->lp, ptr);


Just a note to all of us, maybe we can implement this logic with lpFindCb() in future to avoid performance cost of lpNext() calls.

sundb · 2024-05-28T02:48:18Z

+            int isHashDeleted;
+            int exists = hashTypeExists(key->db, key->value, field->ptr, &isHashDeleted);
+            /* hash-field-expiration is not exposed to modules */
+            serverAssert(isHashDeleted == 0);


if i use hexpire command to create a hash with TTL fields, then operate the hash with RM_HashSet, will thia assertion be triggered?

Regarding this matter, I am seeking input from those experienced with modules:

We likely need to pass the mode/flags of the opened key and respond appropriately.

Should we update the opened key if the key was deleted due to last field expiration?

@MeirShpilraien, perhaps you can assist. Thanks.

tezc · 2024-05-28T08:11:28Z

+        shared.hdel,
+        createStringObject((char*) key, sdslen(key)),
+        createStringObject(field, fieldLen)
+    };


Note: Later, maybe we can add a check to avoid creating these objects if we are not going to propagate anything.

* For replica sake, rewrite commands `H*EXPIRE*` , `HSETF`, `HGETF` to have absolute unix time in msec. * On active-expiration of field, propagate HDEL to replica (`propagateHashFieldDeletion()`) * On lazy-expiration, propagate HDEL to replica (`hashTypeGetValue()` now calls `hashTypeDelete()`. It also takes care to call `propagateHashFieldDeletion()`). * Fix `H*EXPIRE*` command such that if it gets flag `LT` and it doesn’t have any expiration on the field then it will considered as valid condition. Note, replicas doesn’t make any active expiration, and should avoid lazy expiration. On `hashTypeGetValue()` it doesn't check expiration (As long as the master didn’t request to delete the field, it is valid) TODO: * Attach `dbid` to HASH metadata. See [here](redis#13209 (comment)) --------- Co-authored-by: debing.sun <[email protected]>

HFE to support AOF and replicas

26d050a

moticless requested review from oranagra, sundb and tezc May 21, 2024 12:13

fix flaky test

9b5bd8f

sundb reviewed May 21, 2024

View reviewed changes

Comment thread src/aof.c Outdated

sundb reviewed May 22, 2024

View reviewed changes

Comment thread src/server.h Outdated

Comment thread src/t_hash.c Outdated

Comment thread src/t_hash.c

Merge branch 'hash-field-expiry-integ' into hfe-integ-aof-and-replica

ef1b9e9