Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make KEYS can visit expired key in import-source state #1326

Merged
merged 1 commit into from
Nov 27, 2024
+23 −12
Merged

Make KEYS can visit expired key in import-source state #1326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/db.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ robj *dbRandomKey(serverDb *db) {
if (allvolatile && (server.primary_host || server.import_mode) && --maxtries == 0) {
/* If the DB is composed only of keys with an expire set,
* it could happen that all the keys are already logically
* expired in the repilca, so the function cannot stop because
* expired in the replica, so the function cannot stop because
* expireIfNeeded() is false, nor it can stop because
* dictGetFairRandomKey() returns NULL (there are keys to return).
* To prevent the infinite loop we do some tries, but if there
Expand Down Expand Up @@ -1798,7 +1798,13 @@ int keyIsExpiredWithDictIndex(serverDb *db, robj *key, int dict_index) {
/* Check if the key is expired. */
int keyIsExpired(serverDb *db, robj *key) {
int dict_index = getKVStoreIndexForKey(key->ptr);
return keyIsExpiredWithDictIndex(db, key, dict_index);
if (!keyIsExpiredWithDictIndex(db, key, dict_index)) return 0;

/* See expireIfNeededWithDictIndex for more details. */
if (server.primary_host == NULL && server.import_mode) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we use iAmPrimary() here? I am not very clear about the difference between these two.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, i think we should, we probably need to use iAmMaster to replace all the server. primary_host == NULL. The difference is that i guess, when restarting a cluster replica node, its primary_host may be NULL but it is a replica node in cluster role.

Copy link
Member Author

@enjoy-binbin enjoy-binbin Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess it is a small change (but there are too many references that need to look with), i am going to open a new PR for a better review.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i remember @soloestoy had dealt with it before, do you have the old fix (backups) locally that you want to pick up?

if (server.current_client && server.current_client->flag.import_source) return 0;
}
return 1;
}

keyStatus expireIfNeededWithDictIndex(serverDb *db, robj *key, int flags, int dict_index) {
Expand Down
2 changes: 1 addition & 1 deletion src/networking.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3586,7 +3586,7 @@ void clientCommand(client *c) {
"NO-TOUCH (ON|OFF)",
" Will not touch LRU/LFU stats when this mode is on.",
"IMPORT-SOURCE (ON|OFF)",
" Mark this connection as an import source if server.import_mode is true.",
" Mark this connection as an import source if import-mode is enabled.",
" Sync tools can set their connections into 'import-source' state to visit",
" expired keys.",
NULL};
Expand Down
23 changes: 14 additions & 9 deletions tests/unit/expire.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -841,7 +841,7 @@ start_server {tags {"expire"}} {

r set foo1 bar PX 1
r set foo2 bar PX 1
after 100
after 10

assert_equal [r dbsize] {2}

Expand Down Expand Up @@ -879,22 +879,27 @@ start_server {tags {"expire"}} {
assert_equal [r debug set-active-expire 1] {OK}
} {} {needs:debug}

test {RANDOMKEY can return expired key in import mode} {
test {Client can visit expired key in import-source state} {
r flushall

r config set import-mode yes
assert_equal [r client import-source on] {OK}

r set foo1 bar PX 1
r set foo1 1 PX 1
after 10

set client [valkey [srv "host"] [srv "port"] 0 $::tls]
if {!$::singledb} {
$client select 9
}
assert_equal [$client ttl foo1] {-2}
# Normal clients cannot visit expired key.
assert_equal [r get foo1] {}
assert_equal [r ttl foo1] {-2}
assert_equal [r dbsize] 1

# Client can visit expired key when in import-source state.
assert_equal [r client import-source on] {OK}
assert_equal [r ttl foo1] {0}
assert_equal [r get foo1] {1}
assert_equal [r incr foo1] {2}
assert_equal [r randomkey] {foo1}
assert_equal [r scan 0 match * count 10000] {0 foo1}
assert_equal [r keys *] {foo1}

assert_equal [r client import-source off] {OK}
r config set import-mode no
Expand Down
Loading