fix(server): Save element expirations for hash sets & sets #2223
Conversation
src/server/rdb_load.cc
Outdated
|
|
||
| bool is_intset = true; | ||
| bool is_intset = (rdb_type_ == RDB_TYPE_HASH); | ||
| if (len <= SetFamily::MaxIntsetEntries()) { |
There was a problem hiding this comment.
I think the condition (rdb_type_ == RDB_TYPE_HASH && ltrace->blob_count() < SetFamily::MaxIntsetEntries()) will be more clear here.
you will not need the increment logic above
There was a problem hiding this comment.
I do need the increment logic for incrementing the loop, but I moved that below. Otherwise, SG.
src/server/rdb_load.cc
Outdated
| @@ -514,6 +509,8 @@ void RdbLoaderBase::OpaqueObjLoader::CreateSet(const LoadTrace* ltrace) { | |||
| } else { | |||
| bool use_set2 = GetFlag(FLAGS_use_set2) || rdb_type_ == RDB_TYPE_SET_WITH_EXPIRY; | |||
| if (use_set2) { | |||
| LOG_IF(ERROR, !GetFlag(FLAGS_use_set2)) | |||
There was a problem hiding this comment.
please also print the key in the error.
I actually think that in this case we either need to use the other set impl or not load this entry.
What will be the reason for user to disable use_set2 ? if we have a bug there / cuasing a crash. So I think we should respect the flag and not use set2
There was a problem hiding this comment.
If set2 is disabled, we use Redis implementation of set, which does not support field expiration.
What do you think we should do in that case? Drop the expiration data and load everything?
There was a problem hiding this comment.
So we should either not load the key or we can drop the expiration data.. both options looks ok to me
There was a problem hiding this comment.
It's easier to drop the data (I think), so I went with that
And I also fixed an existing bug (iterating an empty set crashes), but honestly I'm not sure how to reproduce it so it might be an unreachable bug
Fixes #2052