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 replica-announced config option #8653

Merged
merged 1 commit into from Mar 30, 2021
Merged

Conversation

fatpat
Copy link
Contributor

@fatpat fatpat commented Mar 15, 2021

The 'sentinel replicas ' command will ignore replicas with
replica-announced set to no.

The goal of disabling the config setting replica-announced is to allow ghost
replicas. The replica is in the cluster, synchronize with its master, can be
promoted to master and is not exposed to sentinel clients. This way, it is
acting as a live backup or living ghost.

In addition, to prevent the replica to be promoted as master, set
replica-priority to 0.

The initial use case was the following:

we have a 3 nodes redis cluster, with 1 master and 2 replicas
we have a 3 nodes sentinel cluster
clients ask sentinel which redis is the master for writes
clients ask sentinel which redis are the replicas for reads
we have real time high load writes to the masters
we have real time high load reads from replicas
we need async, low frequency, heavy read load using complex and CPU consuming LUA scripts.
we want to ensure the async load will not impact performances on the replicas from which the LUA scripts are running
we want another replica to be replicated (#captainObvious) but we don't want it to be master nor to accept requests from normal clients
so we add replica-announced

This PR replace #8437

@fatpat
Copy link
Contributor Author

fatpat commented Mar 15, 2021

@yossigo @hwware @itamarhaber

Copy link
Member

@yossigo yossigo left a comment

Choose a reason for hiding this comment

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

@redis/core-team your approval is needed for this, as this adds a new configuration option and INFO field for Sentinel's use.

src/sentinel.c Outdated Show resolved Hide resolved
tests/sentinel/tests/08-replica-priority.tcl Outdated Show resolved Hide resolved
tests/sentinel/tests/08-replica-priority.tcl Outdated Show resolved Hide resolved
src/sentinel.c Outdated Show resolved Hide resolved
@yossigo yossigo added approval-needed Waiting for core team approval to be merged state:major-decision Requires core team consensus labels Mar 18, 2021
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.

LGTM (only looked at the new config and new info field).

My only concern was if we want to "pollute" redis with more features that are only used by sentinel at this point, but considering this is following the footsteps of replica-priority, i suppose that's fine.

redis.conf Outdated Show resolved Hide resolved
@itamarhaber
Copy link
Member

LGTM

src/sentinel.c Outdated Show resolved Hide resolved
src/sentinel.c Outdated Show resolved Hide resolved
@hwware
Copy link
Collaborator

hwware commented Mar 18, 2021

just want to put some more thoughts of this feature, we can still aware the replica existance if we see "num-slaves" fields in sentinel masters/master command together with sentinel info-cache, the replica just not "offically announce" it. IMHO this shouldn't cause any issue, but bring benefit for debuging purposes. WDYT @yossigo @oranagra @itamarhaber

The 'sentinel replicas <master>' command will ignore replicas with
replica-announced set to no.

The goal of disabling the config setting replica-announced is to allow ghost
replicas. The replica is in the cluster, synchronize with its master, can be
promoted to master and is not exposed to sentinel clients. This way, it is
acting as a live backup or living ghost.

In addition, to prevent the replica to be promoted as master, set
replica-priority to 0.
@oranagra oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Mar 21, 2021
Copy link
Contributor

@madolson madolson left a comment

Choose a reason for hiding this comment

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

API LGTM

@yossigo yossigo added the release-notes indication that this issue needs to be mentioned in the release notes label Mar 30, 2021
@yossigo
Copy link
Member

yossigo commented Mar 30, 2021

@hwware I somehow missed your comment, sorry about that. I lean towards maintaining consistency between the number of reported replicas and the number, and if this proves to be a troubleshooting obstacle have a dedicated counter for these replicas. Hope this makes sense.

@yossigo yossigo merged commit 91f4f41 into redis:unstable Mar 30, 2021
@yossigo
Copy link
Member

yossigo commented Mar 30, 2021

Merged, thank you @fatpat!

@oranagra oranagra mentioned this pull request Apr 19, 2021
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Sep 8, 2021
The 'sentinel replicas <master>' command will ignore replicas with
`replica-announced` set to no.

The goal of disabling the config setting replica-announced is to allow ghost
replicas. The replica is in the cluster, synchronize with its master, can be
promoted to master and is not exposed to sentinel clients. This way, it is
acting as a live backup or living ghost.

In addition, to prevent the replica to be promoted as master, set
replica-priority to 0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants