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

[Feature] Implement CLUSTER FLUSHSLOT command #1133

Open
madolson opened this issue Oct 7, 2024 · 11 comments
Open

[Feature] Implement CLUSTER FLUSHSLOT command #1133

madolson opened this issue Oct 7, 2024 · 11 comments
Assignees
Labels
enhancement New feature or request help wanted External contributions would be appreciated major-decision-approved Major decision approved by TSC team

Comments

@madolson
Copy link
Member

madolson commented Oct 7, 2024

As part of the per-slot dictionary effort, we implemented all of the needed changes so that we could efficiently unlink and free the data in a single slot with lazyfree. This is useful in two cases:

  1. When a primary loses ownership of a slot, it will temporarily freeze while it synchronously frees the slot.
  2. When doing resharding, you may prefer to forcibly delete all the data from a slot and assign it to another node.

The main point where code needs to be updated:

unsigned int delKeysInSlot(unsigned int hashslot) {

Alternative naming conventions (Naming was finalized, leaving for historical purposes):

  1. SFLUSH (presumably slot flush). The S prefix is also used for set, so it may not make the most sense to emulate.
  2. FLUSHSLOT. Since this is mostly be an admin command, it might make sense to attach the cluster.
@madolson madolson added enhancement New feature or request help wanted External contributions would be appreciated labels Oct 7, 2024
@madolson madolson added this to Roadmap Oct 7, 2024
@madolson madolson moved this to Idea in Roadmap Oct 7, 2024
@PingXie PingXie added the major-decision-pending Major decision pending by TSC team label Oct 7, 2024
@PingXie
Copy link
Member

PingXie commented Oct 7, 2024

I like this idea. Added major-decision-pending since this is a new command and +@valkey-io/core-team

@ranshid
Copy link
Member

ranshid commented Oct 7, 2024

@PingXie @madolson In case we will decide to go ahead, we will be happy to take it

@zuiderkwast
Copy link
Contributor

1 and 2 are separate features? I believe 1 is not an API change. It suggest we let it be controlled by the existing lazyfree config lazyfree-lazy-server-del.

@hwware
Copy link
Member

hwware commented Oct 7, 2024

I prefer naming it as CLUSTER FLUSHSLOT, because it only works in cluster mode, and it should run as asynchronous natively, right? Because for the similar command name FLUSHALL, it could run as ASYNC(default) and SYNC.

@madolson madolson changed the title [Feature] Implement FLUSHSLOT command [Feature] Implement CLUSTER FLUSHSLOT command Oct 7, 2024
@madolson madolson added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels Oct 7, 2024
@madolson
Copy link
Member Author

madolson commented Oct 7, 2024

Note from core meeting. We can split this into two PRs, one for command and one for the improved flush behavior.

@madolson madolson removed this from Roadmap Oct 7, 2024
@madolson madolson moved this to Todo in Valkey 8.1 Oct 7, 2024
@wuranxx
Copy link

wuranxx commented Nov 4, 2024

I'd like to contribute this feature. Is this already under development?

@zuiderkwast
Copy link
Contributor

I'd like to contribute this feature. Is this already under development?

@wuranxx Great! Nobody has started this yet, as far as I'm aware. I assigned you to this issue.

@wuranxx
Copy link

wuranxx commented Nov 19, 2024

In #33 , I noticed that @madolson mentioned avoiding mixing data commands and management commands whenever possible. I think cluster flushslot might fall under the category of a data command. Would it be more appropriate to have it as a standalone command, like CLUSTERFLUSHSLOT, instead of a subcommand under cluster? Or FLUSHSLOT? @hwware @zuiderkwast

@zuiderkwast
Copy link
Contributor

If we have some spelled with a space and some without, it will only be confusion. It looks like a spelling error. When you talk about the cluster commands in IRL meetings, how do you pronounce CLUSTERFLUSHSLOT vs CLUSTER FLUSHSLOT? No... I don't want this.

If CLUSTER FLUSHSLOT is rejected, then I suggest FLUSHSLOT only. (FLUSHDB and FLUSHDB don't have any prefix.)

@zuiderkwast
Copy link
Contributor

More importantly, interface changes require a majority decision in the core team and we have made a decision about this feature. If we change it, we need to make a new decision. I don't know the next time we have time to discuss this. Maybe in half a year from now. We are quite overloaded.

@wuranxx
Copy link

wuranxx commented Nov 19, 2024

More importantly, interface changes require a majority decision in the core team and we have made a decision about this feature. If we change it, we need to make a new decision. I don't know the next time we have time to discuss this. Maybe in half a year from now. We are quite overloaded.

Ok, I will use CLUSTER FLUSHSLOT.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted External contributions would be appreciated major-decision-approved Major decision approved by TSC team
Projects
Status: Todo
Development

No branches or pull requests

6 participants