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.

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

Merged
merged 30 commits into from
Feb 6, 2023

Conversation

msotheeswaran-sc
Copy link
Collaborator

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.

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
Copy link
Collaborator

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) {
Copy link
Collaborator

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 {
Copy link
Collaborator

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?

Copy link
Collaborator

@JohnSully JohnSully left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants