From a84cc20aef9e334d2d0dd6727a6ed2ddaa48b532 Mon Sep 17 00:00:00 2001 From: Moti Cohen Date: Tue, 2 Jul 2024 18:22:10 +0300 Subject: [PATCH] HFE - Fix statistic to count also lazy expired and rename INFO params (#13372) * INFO command : rename `hashes_with_expiry_fields` to `subexpiry` * INFO command : rename `expired_hash_fields` to `expired_subkeys` * Fix statistic of `expired_subkeys` to count also lazy expired * Remove TODOs comments leftover in TCL * Fix potential flaky test of rdb load of hash-field-expiration --- src/server.c | 6 ++-- src/server.h | 2 +- src/t_hash.c | 7 ++-- tests/integration/rdb.tcl | 20 +++++++---- tests/support/util.tcl | 1 - tests/unit/type/hash-field-expire.tcl | 49 ++++++++++++++------------- 6 files changed, 48 insertions(+), 37 deletions(-) diff --git a/src/server.c b/src/server.c index 11646e2568..56a17e32aa 100644 --- a/src/server.c +++ b/src/server.c @@ -2524,7 +2524,7 @@ void resetServerStats(void) { server.stat_numcommands = 0; server.stat_numconnections = 0; server.stat_expiredkeys = 0; - server.stat_expired_hash_fields = 0; + server.stat_expired_subkeys = 0; server.stat_expired_stale_perc = 0; server.stat_expired_time_cap_reached_count = 0; server.stat_expire_cycle_time_used = 0; @@ -5877,7 +5877,7 @@ sds genRedisInfoString(dict *section_dict, int all_sections, int everything) { "sync_full:%lld\r\n", server.stat_sync_full, "sync_partial_ok:%lld\r\n", server.stat_sync_partial_ok, "sync_partial_err:%lld\r\n", server.stat_sync_partial_err, - "expired_hash_fields:%lld\r\n", server.stat_expired_hash_fields, + "expired_subkeys:%lld\r\n", server.stat_expired_subkeys, "expired_keys:%lld\r\n", server.stat_expiredkeys, "expired_stale_perc:%.2f\r\n", server.stat_expired_stale_perc*100, "expired_time_cap_reached_count:%lld\r\n", server.stat_expired_time_cap_reached_count, @@ -6124,7 +6124,7 @@ sds genRedisInfoString(dict *section_dict, int all_sections, int everything) { if (keys || vkeys) { info = sdscatprintf(info, - "db%d:keys=%lld,expires=%lld,avg_ttl=%lld,hashes_with_expiry_fields=%lld\r\n", + "db%d:keys=%lld,expires=%lld,avg_ttl=%lld,subexpiry=%lld\r\n", j, keys, vkeys, server.db[j].avg_ttl, hexpires); } } diff --git a/src/server.h b/src/server.h index 25d14ebe5b..bb7f5b6fae 100644 --- a/src/server.h +++ b/src/server.h @@ -1651,7 +1651,7 @@ struct redisServer { long long stat_numcommands; /* Number of processed commands */ long long stat_numconnections; /* Number of connections received */ long long stat_expiredkeys; /* Number of expired keys */ - long long stat_expired_hash_fields; /* Number of expired hash-fields */ + long long stat_expired_subkeys; /* Number of expired subkeys (Currently only hash-fields) */ double stat_expired_stale_perc; /* Percentage of keys probably expired */ long long stat_expired_time_cap_reached_count; /* Early expire cycle stops.*/ long long stat_expire_cycle_time_used; /* Cumulative microseconds used. */ diff --git a/src/t_hash.c b/src/t_hash.c index 45156f46e5..16f51818a3 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -414,6 +414,7 @@ void listpackExExpire(redisDb *db, robj *o, ExpireInfo *info) { break; propagateHashFieldDeletion(db, ((listpackEx *) o->ptr)->key, (char *)((fref) ? fref : intbuf), flen); + server.stat_expired_subkeys++; ptr = lpNext(lpt->lp, ptr); @@ -545,6 +546,7 @@ SetExRes hashTypeSetExpiryListpack(HashTypeSetEx *ex, sds field, if (unlikely(checkAlreadyExpired(expireAt))) { propagateHashFieldDeletion(ex->db, ex->key->ptr, field, sdslen(field)); hashTypeDelete(ex->hashObj, field, 1); + server.stat_expired_subkeys++; ex->fieldDeleted++; return HSETEX_DELETED; } @@ -758,6 +760,7 @@ GetFieldRes hashTypeGetValue(redisDb *db, robj *o, sds field, unsigned char **vs /* delete the field and propagate the deletion */ serverAssert(hashTypeDelete(o, field, 1) == 1); propagateHashFieldDeletion(db, key, field, sdslen(field)); + server.stat_expired_subkeys++; /* If the field is the last one in the hash, then the hash will be deleted */ res = GETF_EXPIRED; @@ -1042,6 +1045,7 @@ SetExRes hashTypeSetExpiryHT(HashTypeSetEx *exInfo, sds field, uint64_t expireAt /* replicas should not initiate deletion of fields */ propagateHashFieldDeletion(exInfo->db, exInfo->key->ptr, field, sdslen(field)); hashTypeDelete(exInfo->hashObj, field, 1); + server.stat_expired_subkeys++; exInfo->fieldDeleted++; return HSETEX_DELETED; } @@ -1833,7 +1837,6 @@ static uint64_t hashTypeExpire(robj *o, ExpireCtx *expireCtx, int updateGlobalHF .itemsExpired = 0}; listpackExExpire(db, o, &info); - server.stat_expired_hash_fields += info.itemsExpired; keystr = ((listpackEx*)o->ptr)->key; } else { serverAssert(o->encoding == OBJ_ENCODING_HT); @@ -2887,7 +2890,7 @@ static ExpireAction onFieldExpire(eItem item, void *ctx) { dictExpireMetadata *dictExpireMeta = (dictExpireMetadata *) dictMetadata(d); propagateHashFieldDeletion(expCtx->db, dictExpireMeta->key, hf, hfieldlen(hf)); serverAssert(hashTypeDelete(expCtx->hashObj, hf, 0) == 1); - server.stat_expired_hash_fields++; + server.stat_expired_subkeys++; return ACT_REMOVE_EXP_ITEM; } diff --git a/tests/integration/rdb.tcl b/tests/integration/rdb.tcl index f528097f4c..2d3eb0e09d 100644 --- a/tests/integration/rdb.tcl +++ b/tests/integration/rdb.tcl @@ -442,15 +442,21 @@ start_server [list overrides [list "dir" $server_path]] { restart_server 0 true false wait_done_loading r - assert_equal [lsort [r hgetall key]] "1 2 3 a b c" + # Never be sure when active-expire kicks in into action + wait_for_condition 100 10 { + [lsort [r hgetall key]] == "1 2 3 a b c" + } else { + fail "hgetall of key is not as expected" + } + assert_equal [r hpexpiretime key FIELDS 3 a b c] {2524600800000 65755674080852 -1} assert_equal [s rdb_last_load_keys_loaded] 1 - # wait until expired_hash_fields equals 2 + # wait until expired_subkeys equals 2 wait_for_condition 10 100 { - [s expired_hash_fields] == 2 + [s expired_subkeys] == 2 } else { - fail "Value of expired_hash_fields is not as expected" + fail "Value of expired_subkeys is not as expected" } } } @@ -562,9 +568,9 @@ foreach {type lp_entries} {listpack 512 dict 0} { # wait at most 2 secs to make sure 'c' and 'd' will active-expire wait_for_condition 20 100 { - [s expired_hash_fields] == 2 + [s expired_subkeys] == 2 } else { - fail "expired hash fields is [s expired_hash_fields] != 2" + fail "expired hash fields is [s expired_subkeys] != 2" } assert_equal [s rdb_last_load_keys_loaded] 1 @@ -597,7 +603,7 @@ foreach {type lp_entries} {listpack 512 dict 0} { after 500 assert_equal [s rdb_last_load_keys_loaded] 1 - assert_equal [s expired_hash_fields] 0 + assert_equal [s expired_subkeys] 0 # hgetall will lazy expire fields, so it's only called after the stat asserts assert_equal [lsort [r hgetall key]] "1 2 5 6 a b e f" diff --git a/tests/support/util.tcl b/tests/support/util.tcl index 09e5b1e3b4..8ebd1e3620 100644 --- a/tests/support/util.tcl +++ b/tests/support/util.tcl @@ -293,7 +293,6 @@ proc findKeyWithType {r type} { proc createComplexDataset {r ops {opt {}}} { set useexpire [expr {[lsearch -exact $opt useexpire] != -1}] - # TODO: Remove usehexpire on next commit, when RDB will support replication set usehexpire [expr {[lsearch -exact $opt usehexpire] != -1}] if {[lsearch -exact $opt usetag] != -1} { diff --git a/tests/unit/type/hash-field-expire.tcl b/tests/unit/type/hash-field-expire.tcl index b6dd58043c..d753ca8b8c 100644 --- a/tests/unit/type/hash-field-expire.tcl +++ b/tests/unit/type/hash-field-expire.tcl @@ -19,12 +19,12 @@ set P_OK 1 ############################### AUX FUNCS ###################################### -proc get_hashes_with_expiry_fields {r} { +proc get_stat_subexpiry {r} { set input_string [r info keyspace] set hash_count 0 foreach line [split $input_string \n] { - if {[regexp {hashes_with_expiry_fields=(\d+)} $line -> value]} { + if {[regexp {subexpiry=(\d+)} $line -> value]} { return $value } } @@ -163,9 +163,6 @@ start_server {tags {"external:skip needs:debug"}} { } test "Lazy Expire - fields are lazy deleted ($type)" { - - # TODO remove the SELECT once dbid will be embedded inside dict/listpack - r select 0 r debug set-active-expire 0 r del myhash @@ -349,10 +346,12 @@ start_server {tags {"external:skip needs:debug"}} { test "Test HRANDFIELD deletes all expired fields ($type)" { r debug set-active-expire 0 r flushall + r config resetstat r hset myhash f1 v1 f2 v2 f3 v3 f4 v4 f5 v5 r hpexpire myhash 1 FIELDS 2 f1 f2 after 5 assert_equal [lsort [r hrandfield myhash 5]] "f3 f4 f5" + assert_equal [s expired_subkeys] 2 r hpexpire myhash 1 FIELDS 3 f3 f4 f5 after 5 assert_equal [lsort [r hrandfield myhash 5]] "" @@ -476,39 +475,48 @@ start_server {tags {"external:skip needs:debug"}} { r hset h5 1 1 2 22 3 333 4 4444 5 55555 r hset h6 01 01 02 02 03 03 04 04 05 05 06 06 r hset h18 01 01 02 02 03 03 04 04 05 05 06 06 07 07 08 08 09 09 10 10 11 11 12 12 13 13 14 14 15 15 16 16 17 17 18 18 - r hpexpire h1 100 NX FIELDS 1 01 - r hpexpire h2 100 NX FIELDS 1 01 - r hpexpire h2 100 NX FIELDS 1 02 - r hpexpire h3 100 NX FIELDS 1 01 - r hpexpire h4 100 NX FIELDS 1 2 - r hpexpire h5 100 NX FIELDS 1 3 - r hpexpire h6 100 NX FIELDS 1 05 - r hpexpire h18 100 NX FIELDS 17 01 02 03 04 05 06 07 08 09 10 11 12 13 14 15 16 17 + r hpexpire h1 1 NX FIELDS 1 01 + r hpexpire h2 1 NX FIELDS 1 01 + r hpexpire h2 1 NX FIELDS 1 02 + r hpexpire h3 1 NX FIELDS 1 01 + r hpexpire h4 1 NX FIELDS 1 2 + r hpexpire h5 1 NX FIELDS 1 3 + r hpexpire h6 1 NX FIELDS 1 05 + r hpexpire h18 1 NX FIELDS 17 01 02 03 04 05 06 07 08 09 10 11 12 13 14 15 16 17 - after 150 + after 5 # Verify HDEL not ignore expired field. It is too much overhead to check # if the field is expired before deletion. assert_equal [r HDEL h1 01] "1" # Verify HGET ignore expired field + r config resetstat assert_equal [r HGET h2 01] "" + assert_equal [s expired_subkeys] 1 assert_equal [r HGET h2 02] "" + assert_equal [s expired_subkeys] 2 assert_equal [r HGET h3 01] "" assert_equal [r HGET h3 02] "02" assert_equal [r HGET h3 03] "03" + assert_equal [s expired_subkeys] 3 # Verify HINCRBY ignore expired field assert_equal [r HINCRBY h4 2 1] "1" + assert_equal [s expired_subkeys] 4 assert_equal [r HINCRBY h4 3 1] "100" # Verify HSTRLEN ignore expired field assert_equal [r HSTRLEN h5 3] "0" + assert_equal [s expired_subkeys] 5 assert_equal [r HSTRLEN h5 4] "4" assert_equal [lsort [r HKEYS h6]] "01 02 03 04 06" + assert_equal [s expired_subkeys] 5 # Verify HEXISTS ignore expired field assert_equal [r HEXISTS h18 07] "0" + assert_equal [s expired_subkeys] 6 assert_equal [r HEXISTS h18 18] "1" # Verify HVALS ignore expired field assert_equal [lsort [r HVALS h18]] "18" + assert_equal [s expired_subkeys] 6 # Restore to support active expire r debug set-active-expire 1 } @@ -898,14 +906,14 @@ start_server {tags {"external:skip needs:debug"}} { r hset myhash f1 v1 f2 v2 f3 v3 f4 v4 f5 v5 r hpexpire myhash 100 FIELDS 3 f1 f2 f3 - assert_match [get_hashes_with_expiry_fields r] 1 + assert_match [get_stat_subexpiry r] 1 r hset myhash2 f1 v1 f2 v2 f3 v3 f4 v4 f5 v5 - assert_match [get_hashes_with_expiry_fields r] 1 + assert_match [get_stat_subexpiry r] 1 r hpexpire myhash2 100 FIELDS 3 f1 f2 f3 - assert_match [get_hashes_with_expiry_fields r] 2 + assert_match [get_stat_subexpiry r] 2 wait_for_condition 50 50 { - [get_hashes_with_expiry_fields r] == 0 + [get_stat_subexpiry r] == 0 } else { fail "Hash field expiry statistics failed" } @@ -1039,11 +1047,8 @@ start_server {tags {"external:skip needs:debug"}} { r hpexpire h1 100000 NX FIELDS 3 f3 f4 f5 r hexpire h1 100000 FIELDS 1 f6 - # Verify HRANDFIELD deletes expired fields and propagates it r hset h2 f1 v1 f2 v2 r hpexpire h2 1 FIELDS 2 f1 f2 - after 5 - assert_equal [r hrandfield h4 2] "" after 200 assert_aof_content $aof { @@ -1060,8 +1065,6 @@ start_server {tags {"external:skip needs:debug"}} { } array set keyAndFields1 [dumpAllHashes r] - # Let some time pass and reload data from AOF - after 2000 r debug loadaof array set keyAndFields2 [dumpAllHashes r]