-
Notifications
You must be signed in to change notification settings - Fork 947
Implement bigkey real time scan feature #2819
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.
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
base: unstable
Are you sure you want to change the base?
Conversation
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
ranshid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hwware thank you for picking up on this major important functionality!
I only gave it a short run through but I think we should discuss the intrusiveness of this solution.
This is basically introducing a modification for every mutative command for the objects we track. this might be hard to maintain it in the future. For example, hash objects have relatively large mutative API and they also support field evictions which should also be addressed. I guess your motivation was driven by performance considerations (right?) in that case I would love if we could discuss alternatives. For example:
I would start from a module optional implementation. is it possible to hook to the keyspace notifications and update the bigkeys tracking following notifications?
| while ((ln = listNext(&li)) != NULL) { | ||
| bigkeyEntry *node = ln->value; | ||
| if (sdscmp(node->key->ptr, keyobj->ptr) != 0) continue; | ||
| decrRefCount(node->key); | ||
| listDelNode(process_list, ln); | ||
| zfree(node); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems somewhat inefficient. Maybe we should try and include a b+ tree structure that will maintain the order and the ability to add/remove the object.
Another thing is the rename command. is it possible we might be renaming a key and it's metadata is not updated?
| createIntConfig("rdma-port", NULL, MODIFIABLE_CONFIG, 0, 65535, server.rdma_ctx_config.port, 0, INTEGER_CONFIG, NULL, updateRdmaPort), | ||
| createIntConfig("rdma-rx-size", NULL, IMMUTABLE_CONFIG, 64 * 1024, 16 * 1024 * 1024, server.rdma_ctx_config.rx_size, 1024 * 1024, INTEGER_CONFIG, NULL, NULL), | ||
| createIntConfig("rdma-completion-vector", NULL, IMMUTABLE_CONFIG, -1, 1024, server.rdma_ctx_config.completion_vector, -1, INTEGER_CONFIG, NULL, NULL), | ||
| createIntConfig("string-memory-use", NULL, IMMUTABLE_CONFIG, 0, INT_MAX, server.string_memory_use, 10240, INTEGER_CONFIG, NULL, NULL), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string_memory_use does not sound like the thing we are tracking right? we are tracking for cardinality, so maybe "big-key-object-size-min"? or "big-key-object-card-min"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I should delete it, it exists in old design.
Thanks for your comments. Indeed, this feature involves every mutative command and one of the important issue we would like to discuss in if we should implement it in a separated module instead of in the Valkey core. I think maybe I will add it in the late of November core meeting to discuss. |
Same function as valkey-cli --bigkey, but clients do not need to scan whole keyspace separately