-
Notifications
You must be signed in to change notification settings - Fork 583
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
Add support to notify modules of keys loaded by flash on startup #536
Conversation
keydb.conf
Outdated
@@ -2088,3 +2088,6 @@ active-client-balancing yes | |||
# disk space or any other I/O error KeyDB will instead use memory. | |||
# | |||
# blob-support false | |||
|
|||
# Use this config to notify modules of every key loaded from flash on startup (warning: can make startup much slower) | |||
# module-notify-flash-load yes |
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.
If this is part of the module API why do we need a config to enable it?
src/server.cpp
Outdated
@@ -6953,6 +6953,18 @@ void loadDataFromDisk(void) { | |||
|
|||
if (g_pserver->m_pstorageFactory) | |||
{ | |||
if (g_pserver->config_notify_flash_load) { |
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.
can't you gate this on whether a module is loaded.
src/server.cpp
Outdated
moduleFireServerEvent(REDISMODULE_EVENT_LOADING, REDISMODULE_SUBEVENT_LOADING_FLASH_START, NULL); | ||
for (int idb = 0; idb < cserver.dbnum; ++idb) { | ||
auto spsnapshot = g_pserver->db[idb]->CloneStorageCache(); | ||
spsnapshot->enumerate([idb](const char *rgchKey, size_t cchKey, const void *, size_t) -> bool { |
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.
we already enumerate when setting up the StorageCache, can you do this there so we don't double startup time?
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.
If you can tell by the delay this change makes me nervous but I don't see any issues at the moment. We'll need to make sure we test this scenario more before release.
Currently if flash is enabled there is no rdb load, however some modules expect to be notified of keys loaded from rdb, so add the option to have modules notified of keys loaded from flash.