Unsubscribe all clients from replica for shard channel if the master ownership changes#12577

Merged

madolson merged 9 commits intoredis:unstablefrom

hpatro:fix-shardpubsub-cluster-replicate

Oct 13, 2023

Contributor

hpatro commented Sep 14, 2023 •

edited by madolson

Loading

Fixes #12558

Release notes
Fix a bug where a replica would continue to serve slots it doesn't have data for after switching masters which owns different slots.


          Unsubscribe all clients from replica for shard channel if the master …

…ownership changes

hpatro requested review from enjoy-binbin, madolson and zuiderkwast

September 14, 2023 16:31

zuiderkwast reviewed

View reviewed changes

Contributor

zuiderkwast left a comment

It seems reasonable to unsubscribe clients from shard channels when the shard changes.

Do we ever do this for regular subscribe channels and patterns?

I'm not sure all clients can handle a 'sunsubscribe' message without first having sent a SUNSUBSCRIBE command. Maybe not even redis-cli can handle this. We should check.

We need to clarify in the docs that the client can receive 'sunsubscribe' messages even when they haven't sent SUNSUBSCRIBE.

src/cluster.c Outdated Show resolved Hide resolved

Contributor Author

hpatro commented Sep 15, 2023 •

edited

Loading

Do we ever do this for regular subscribe channels and patterns?

No, not that I'm aware of.

I'm not sure all clients can handle a 'sunsubscribe' message without first having sent a SUNSUBSCRIBE command. Maybe not even redis-cli can handle this. We should check.

We need to clarify in the docs that the client can receive 'sunsubscribe' messages even when they haven't sent SUNSUBSCRIBE.

This is already the behavior when individual slots get migrated from one node to another. Node migration scenario wasn't covered. Yeah, we could document this for client(s) to take action (possibly disconnect and reconnect the client) and improve redis-cli first. I will follow up on both of these.


          Address feedback

6fcec05

zuiderkwast reviewed

View reviewed changes

src/cluster.c Outdated Show resolved Hide resolved

hpatro mentioned this pull request

[BUG] redis-cli doesn't auto exit from subscribed mode on reaching zero subscription #12592

Open


          Avoid disconnection if the node is still within the same shard

a382682

zuiderkwast reviewed

View reviewed changes

src/cluster.c Outdated Show resolved Hide resolved

madolson reviewed

View reviewed changes

src/cluster.c Outdated Show resolved Hide resolved

madolson reviewed

View reviewed changes

src/cluster.c Outdated Show resolved Hide resolved

madolson reviewed

View reviewed changes

src/cluster.c Outdated Show resolved Hide resolved

madolson reviewed

View reviewed changes

src/cluster.c Outdated Show resolved Hide resolved

hpatro and others added 5 commits

October 11, 2023 21:03


          Merge branch 'unstable' into fix-shardpubsub-cluster-replicate

9bc21f7


          Fix grammatical mistakes

763ebeb


          Address feedback

40b4a29


          Merge branch 'unstable' into fix-shardpubsub-cluster-replicate

a0cf628


          Fix build

95ceb10

madolson reviewed

View reviewed changes

src/cluster.c Show resolved Hide resolved


          Early exit on no shard channel subscription

f4365ca

Co-authored-by: Madelyn Olson <[email protected]>

madolson approved these changes

View reviewed changes

Contributor

madolson commented Oct 12, 2023

Not back porting immediately as there is no immediate need. If it comes up, we can backport and release.

madolson added the release-notes label

enjoy-binbin approved these changes

View reviewed changes

madolson merged commit b784c53 into redis:unstable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels