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

Avoid expiration and eviction during data syncing #13077

Open
wants to merge 5 commits into
base: unstable
Choose a base branch
from

Conversation

lyq2333
Copy link
Contributor

@lyq2333 lyq2333 commented Feb 21, 2024

When we sync data from the source Redis to the destination Redis using some sync tools like redis-shake, the destination Redis think it's a master and can perform expiration and eviction, which may cause data corruption like what @oranagra said in #9760 (reply in thread).

In standalone mode, we can use writable replica to simplify the sync process. However, in cluster mode, we still need a sync tool to help us transfer the source data to the destination. The sync tool usually work as a normal client so the destination works as a master which keep expiration and eviction.

In this PR, we add a new mode for master named 'pseudo-replica'. In this mode, server stop expiration and eviction just like a replica. Notice that this mode exists only in sync state to avoid data inconsistency caused by expiration and eviction.

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.

i didn't take a deep look at this PR yet, but we do already have this in our fork, so maybe it's easier and safer to PR that than review another implementation.
@YaacovHazan FYI.

Comment on lines 1282 to 1287
} else if (!strcasecmp(c->argv[j]->ptr,"pseudo-replica")) {
/* REPLCONF PSEUDO-REPLICA is used to set this replica
* into 'pseudo-replica' mode to avoid eviction and expiration.
* This is used for sync tools which eviction and expiration may
* cause the data corruption. */
long pseudo_replica = 0;
Copy link
Member

Choose a reason for hiding this comment

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

REPLCONF is used to set the client state of the calling client, not the server state.
this means that pseudo-replica should be a config (config.c).
And REPLCONF should have a pseudo-master to set the current connection behave like a master.
if we tie them together, it may mean we need to reset the server state when that client drops, which not necessarily desired since it may soon re-connect and we still don't want eviction to kick in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review. I have separated these two logics.

@lyq2333
Copy link
Contributor Author

lyq2333 commented Feb 22, 2024

i didn't take a deep look at this PR yet, but we do already have this in our fork, so maybe it's easier and safer to PR that than review another implementation. @YaacovHazan FYI.

@oranagra Very glad that you have a plan to PR that :). Before that, I'm willing to continue this work.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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

Successfully merging this pull request may close these issues.

None yet

3 participants