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

DISCARD doesn't always discard #25

Closed
aphyr opened this issue Mar 13, 2020 · 2 comments
Closed

DISCARD doesn't always discard #25

aphyr opened this issue Mar 13, 2020 · 2 comments

Comments

@aphyr
Copy link
Collaborator

aphyr commented Mar 13, 2020

Calling MULTI puts a connection into a batch state, but calling DISCARD doesn't necessary end that batch state. In particular, DISCARD frequently fails on followers without proxies enabled, returning MOVED, NOLEADER, or NOTLEADER . When this happens, the connection is still usable, but subsequent requests will execute within a batch context, rather than independently. This leads to correctness problems!

This makes it difficult for clients to reliably use MULTI, because a transaction wrapper (e.g. (with-txn conn (do-some-stuff-with conn))) has no way to guarantee that it has returned the connection to a non-batch context by the end of the block, unless it:

  1. Closes the connection altogether
  2. Blocks and retries (possibly indefinitely) until the connection accepts a DISCARD operation.
  3. Sets some kind of mutable state, ensuring that any future use of that connection make sure to DISCARD prior to making a request.

MULTI & DISCARD modify in-memory state purely on the listening server, right? Would it be possible to guarantee that DISCARD always succeeds when a server is reachable?

@yossigo
Copy link
Collaborator

yossigo commented Mar 17, 2020

This goes beyond DISCARD, so I think it's worth a more thorough explanation.

Historically, Redis never had to take extra steps to provide MULTI/EXEC isolation because commands execute serially in a single thread. So fundamentally MULTI is a mechanism to bundle commands together and execute them atomically at the time of EXEC.

Redis Raft attempts to apply the Raft cluster state checks to this process, but that is basically wrong. The correct behavior is to follow the traditional Redis way of doing this and handle cluster state only at the time of EXEC. This guarantees that:

  1. MULTI never fails.
  2. Everything following it always gets +QUEUED (or, at least does not fail due to cluster state).
  3. DISCARD never fails.
  4. EXEC will act on the bundled set of commands depending on context; i.e. execute through Raft if it is a leader; reply with -MOVED if is a follower and proxy is off; send as a bundle to the leader if it is a follower and proxy is on; reply with various errors if necessary, etc.

This behavior also maintains better compatibility with clients, that may be implemented based on some of these assumptions.

@aphyr
Copy link
Collaborator Author

aphyr commented Mar 17, 2020

Yeah, I think this makes total sense.

@yossigo yossigo closed this as completed Apr 6, 2020
sjpotter added a commit to sjpotter/redisraft that referenced this issue Oct 7, 2021
…sLabs#25)

this commit changes libraft's from using the DEMOTE_NODE log entry which would then initiate a REMOVE_NODE log entry on the DEMOTE's commit, to only using a REMOVE.  

* add the removal of the active node flag on append of REMOVE and (add back if reverted)
* modify is_voting to check for active state as well

this enables simplifying appending and reverting nodes, as we shouldn't be touching voting state, (as a removing node might not be voting yet), only its active state.  and as the is_voting check also now checks for active state, simplifies a lot of code that manually tested both.

* add a test for reversion of a non voting node removal

previously, the code could not handle demoting/removing a non voting node, as demote would want to unset the voting flag (which a non voting node obviously doesn't have), and would therefore fail an assertion.

with the new code, we no longer unset the voting flag in removal, and this test make sure both the is_voting and is_active return as expected through a process of add/removal/revert
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants