Make KEYS can visit expired key in import-source state #1326
Conversation
After valkey-io#1185, a client in import-source state can visit expired key both in read commands and write commands, this commit handle keyIsExpired function to handle import-source state as well, so KEYS can visit the expired key. This is not particularly important, but it ensures the definition, also doing some cleanup around the test, verified that the client can indeed visit the expired key. Signed-off-by: Binbin <binloveplay1314@qq.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #1326 +/- ##
============================================
- Coverage 70.68% 70.57% -0.11%
============================================
Files 115 115
Lines 63177 63179 +2
============================================
- Hits 44657 44589 -68
- Misses 18520 18590 +70
|
|
I guess an importing client will not use KEYS and SCAN, but I agree, it's good to ensure the contract that this client can read expired keys. |
| if (!keyIsExpiredWithDictIndex(db, key, dict_index)) return 0; | ||
|
|
||
| /* See expireIfNeededWithDictIndex for more details. */ | ||
| if (server.primary_host == NULL && server.import_mode) { |
There was a problem hiding this comment.
Shall we use iAmPrimary() here? I am not very clear about the difference between these two.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
i remember @soloestoy had dealt with it before, do you have the old fix (backups) locally that you want to pick up?
In valkey-io#1326 we make KEYS can visit expired key in import-source state by updating keyIsExpired to check for import mode. But after valkey-io#1205, we now use keyIsExpiredWithDictIndex to optimize and remove the redundant dict_index, and keyIsExpiredWithDictIndex does not handle this logic. In this commit, we handle keyIsExpiredWithDictIndex to make it check for import mode as well so that KEYS can visit the expired key. Signed-off-by: Binbin <binloveplay1314@qq.com>
) In #1326 we make KEYS can visit expired key in import-source state by updating keyIsExpired to check for import mode. But after #1205, we now use keyIsExpiredWithDictIndex to optimize and remove the redundant dict_index, and keyIsExpiredWithDictIndex does not handle this logic. In this commit, we handle keyIsExpiredWithDictIndex to make it check for import mode as well so that KEYS can visit the expired key. Signed-off-by: Binbin <binloveplay1314@qq.com>
After #1185, a client in import-source state can visit expired
key both in read commands and write commands, this commit handle
keyIsExpired function to handle import-source state as well, so
KEYS can visit the expired key.
This is not particularly important, but it ensures the definition,
also doing some cleanup around the test, verified that the client
can indeed visit the expired key.