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

Change FLUSHALL/FLUSHDB SYNC to run as blocking ASYNC #13167

Merged
merged 2 commits into from Apr 2, 2024

Conversation

moticless
Copy link
Collaborator

Overview

Users utilize the FLUSHDB SYNC and FLUSHALL SYNC commands for a variety of
reasons. The main issue with this command is that if the database becomes substantial
in size, the server will be unresponsive for an extended period. Other than freezing
application traffic, this may also lead some clients making incorrect judgments about
the server's availability. For instance, a watchdog may erroneously decide to terminate
the process, resulting in potential adverse outcomes.

While a FLUSH* ASYNC can address these issues, it might not be used for two
reasons: firstly, it's not the default, and secondly, in some cases, the client issuing
the flush wants to wait for its completion before repopulating the database.

Between the option of triggering FLUSH* asynchronously in the background without
indication for completion versus running it synchronously in the foreground by the
main thread, there is another more appealing option. We can block the client
that requested the flush, execute the flush command in the background, and once done,
unblock the client and return notification for completion. This approach ensures
the server remains responsive to other clients, and the blocked client receives the
expected response only after the flush operation has been successfully carried out.

Implementation details

Instead of defining yet another flavor to the flush command, we can modify
FLUSHALL SYNC and FLUSHDB SYNC always run in this new mode.

Extending BIO Threads capabilities

Today jobs that are carried out by BIO threads don't have the capability to indicate
completion to the main thread. We can add this infrastructure by having an additional
dummy job, coined as completion-job, that eventually will be written by BIO threads
to a response-queue. The main thread will take care to consume items from the
response-queue and call the provided callback function of each completion-job.

FLUSH* SYNC to run as blocking ASYNC

Command FLUSH* SYNC will be modified to create one or more async jobs to flush
DB(s) and afterward will push additional completion-job request. By sending the
completion job request only at the end, the main thread will be called back only
after all the preceding jobs completed their task in the background. During that
time, the client of the command is suspended and marked as BLOCKED_LAZYFREE
whereas any other client will be able to communicate with the server without any
issue.

@CLAassistant
Copy link

CLAassistant commented Mar 24, 2024

CLA assistant check
All committers have signed the CLA.

src/bio.c Outdated Show resolved Hide resolved
src/bio.c Show resolved Hide resolved
src/bio.c Show resolved Hide resolved
src/bio.h Show resolved Hide resolved
src/bio.c Show resolved Hide resolved
src/bio.c Outdated Show resolved Hide resolved
src/bio.c Outdated Show resolved Hide resolved
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the record, I reviewed this code in an internal fork in the past.
and discussed the code copying / cherry pick procedure with Moti.
so conceptually approving it when sundb does.

src/bio.h Show resolved Hide resolved
@moticless moticless merged commit 4df0379 into redis:unstable Apr 2, 2024
14 checks passed
@moticless moticless deleted the flush-syn-as-block-async branch April 2, 2024 12:09
sazzad16 added a commit to redis/jedis that referenced this pull request Apr 4, 2024
Due to redis/redis#13167 

* Fix JedisClusterClientSideCacheTest

* Fix JedisSentineledClientSideCacheTest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants