Skip to content

bugfix#3450

Closed
andyli029 wants to merge 2 commits into
redis:3.0from
andyli029:3.0
Closed

bugfix#3450
andyli029 wants to merge 2 commits into
redis:3.0from
andyli029:3.0

Conversation

@andyli029

Copy link
Copy Markdown
Contributor

No description provided.

@andyli029 andyli029 changed the title Forbid the command(cluster setslot) to slave bugfix Sep 19, 2016
@yoav-steinberg

yoav-steinberg commented Jul 29, 2021

Copy link
Copy Markdown
Contributor

Thanks. Please rebase and remove the AUTH addition to MIGRATE as it has been added in 3ce1c28.
Also make sure you have only one issue per PR and fix the title accordingly ("bugfix" isn't descriptive enough).

@yossigo does this 6910113 fix seem correct?

@yossigo

yossigo commented Jul 29, 2021

Copy link
Copy Markdown
Collaborator

@yoav-steinberg I think it's correct. @ushachar what do you think?

@ushachar

Copy link
Copy Markdown
Contributor

@yossigo There's no need for this, there's a nodeIsSlave check at the beginning of the handling for all SETSLOT subcommands...

@madolson

Copy link
Copy Markdown
Contributor

There are quite a few more issues here as well. For 1, this is merging against Redis 3.0, which is definitely no longer supported. Some of this code is redundant. We probably just want to replace this with a new PR with the couple of fixes that are still relevant.

@yoav-steinberg

yoav-steinberg commented Aug 3, 2021

Copy link
Copy Markdown
Contributor

@yossigo There's no need for this, there's a nodeIsSlave check at the beginning of the handling for all SETSLOT subcommands...

The call to nodeIsSalve() at the top checks myself, but there are variants of this command (IMPORTING, MIGRATING, NODE) that receive a node id as and argument. The fix checks that argument. That's why it seemed relevant to me.

@madolson

madolson commented Aug 4, 2021

Copy link
Copy Markdown
Contributor

@yoav-steinberg I agree I think those are useful, I'll open another PR with just those fixes.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants