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.

Sign up for GitHub

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

Jump to bottom

storage{,/engine}: separate Raft application side effects, remove (Batch).Defer() #6286

Closed
tbg opened this issue Apr 25, 2016 · 2 comments
Closed

storage{,/engine}: separate Raft application side effects, remove (Batch).Defer() #6286

tbg opened this issue Apr 25, 2016 · 2 comments
Assignees
@tbg

Comments

@tbg
Copy link
Member

tbg commented Apr 25, 2016 •
edited
Loading

See #6166.

Separate the side effects of Raft applications from *Replica and clearly separate the parts which can be moved pre-proposal as well as the parameters required for the post-proposal part. Most of these should come up naturally when trying to remove the (*Replica) receiver on most of the ReplicaCommands (LeaderLease, Put, ...) and manually going through the rest of the call stack up to processRaftCommand.
@tbg tbg self-assigned this Jun 13, 2016
@tbg tbg changed the title storage: separate Raft application side effects storage{,/engine}: separate Raft application side effects, remove (Batch).Defer() Jun 13, 2016
@tbg
Copy link
Member Author

tbg commented Jun 13, 2016 •
edited
Loading

All Raft command side effects make use of (Batch).Defer(), and so removing it is the first step here (there are no noteworthy uses elsewhere). Instead, Raft commands should return a side effect (generalizing the "skipped intents" slice we return from all read operations). Side effects come roughly in two flavors: Those that mutate the replica state (and want to run on all replicas atomically with the batch) and those which trigger auxiliary actions (usually, but not always, on the leader - examples are adding replicas to queues, gossipping, or transferring Raft leadership). Side effects must be serialized, so they're going to be protos and they will be carrying some data. Changes to side effects are going to be harder to migrate: naively they need stop-the-world, though one could hope to perform a "freeze" which only causes commands with side effects to fail.

The second step is tightening access to *Replica. The state machine functions should only be allowed to access the state whose consistency is guaranteed by Raft. We're fairly good about this, so this is mostly renaming and restructuring, but there are isolated bits of code that will need special treatment, for example that introduced in #3175.

This was referenced Jul 13, 2016
Merged
Closed
Closed
Closed
tbg added a commit to tbg/cockroach that referenced this issue Jul 19, 2016
@tbg
storage: extract post-commit portion of split
50b4368 
This is the first step towards formalizing the split trigger as a side effect
in the sense of cockroachdb#6286. With proposed-evaluated KV, we will have to attach
a small amount of metadata to the WriteBatch containing the SplitTrigger based
from which we have to deduce the necessary actions.

I have more WIP which contains plumbing-type work, but have decided to take
a step back and bring these changes in in small increments as a plethora of
roadblocks is to be expected and cockroachdb#6144 may influence (and/or be influenced by)
this work.

Code movement only (and changing a few words around the boundary of the move)
in this commit.
@tbg tbg mentioned this issue Jul 19, 2016
Merged
tbg added a commit to tbg/cockroach that referenced this issue Jul 25, 2016
@tbg
storage: extract post-commit portion of split
ad991e3 
This is the first step towards formalizing the split trigger as a side effect
in the sense of cockroachdb#6286. With proposed-evaluated KV, we will have to attach
a small amount of metadata to the WriteBatch containing the SplitTrigger based
from which we have to deduce the necessary actions.

I have more WIP which contains plumbing-type work, but have decided to take
a step back and bring these changes in in small increments as a plethora of
roadblocks is to be expected and cockroachdb#6144 may influence (and/or be influenced by)
this work.

Code movement only (and changing a few words around the boundary of the move)
in this commit.
tbg added a commit to tbg/cockroach that referenced this issue Jul 26, 2016
@tbg
storage: hoist split side effects
76fd805 
Previously, a committed split would have the Store process that split
via a `batch.Defer`. As part of cockroachdb#6286, this `batch.Defer` was removed
and explicit metainformation is now passed up the callchain up to
`(*Replica).processRaftCommand`, where it is translated to calls to
the Store.

A similar mechanism was previously used for passing up skipped intents,
and that mechanism has been folded into this new one (with more side
effects to be extracted in future work).
tbg added a commit to tbg/cockroach that referenced this issue Jul 26, 2016
@tbg
storage: hoist split side effects
cbf4e98 
Previously, a committed split would have the Store process that split
via a `batch.Defer`. As part of cockroachdb#6286, this `batch.Defer` was removed
and explicit metainformation is now passed up the callchain up to
`(*Replica).processRaftCommand`, where it is translated to calls to
the Store.

A similar mechanism was previously used for passing up skipped intents,
and that mechanism has been folded into this new one (with more side
effects to be extracted in future work).
tbg added a commit to tbg/cockroach that referenced this issue Jul 27, 2016
@tbg
storage: hoist split side effects
42fe299 
Previously, a committed split would have the Store process that split
via a `batch.Defer`. As part of cockroachdb#6286, this `batch.Defer` was removed
and explicit metainformation is now passed up the callchain up to
`(*Replica).processRaftCommand`, where it is translated to calls to
the Store.

A similar mechanism was previously used for passing up skipped intents,
and that mechanism has been folded into this new one (with more side
effects to be extracted in future work).
tbg added a commit to tbg/cockroach that referenced this issue Aug 2, 2016
@tbg
storage: refactor for proposer-eval'ed kv
2896b8c 
Starting from `applyRaftCommand`, refactor in preparation for cockroachdb#6286 (which at
this point is "almost" closed, missing only a second audit and some refactoring
which makes it *impossible* for those methods to do anything but use the
batch).

Due to the subtlety of this change, it should be a prime suspect for anomalies
in the near future. The movement of code may have changed some test semantics
in subtle ways, in the most dangerous case letting them pass while hiding a
regression. However, most of the changes were relatively mechanical and
appropriate care has been taken.

Prepare `applyRaftCommand` for cockroachdb#6290. After that issue, `applyRaftCommand`
will operate on a supplied `WriteBatch` and an associated protobuf containing
information necessary to execute all relevant side effects.

The `WriteBatch` will be constructed on the lease holder (by calling some new
method which will contain much of what is now the part of `applyRaftCommand`
leading down to `executeWriteBatch`).

The heavy lifting here is removing all spurious side effects from the state
machine methods multiplexed to via `(*Replica).executeCmd`.

In particular, in this change we

- remove a first handful of `(*Batch).Defer`s, introduced a type
  `proposalResult` which along with the `WriteBatch` must contain all the
  information necessary for applying the command. Sprinkled TODOs liberally to
  give a general sense of what's to be done.

- hoist communicating MVCCStats delta to store and leave a TODO for the
  handling of `ReplicaCorruptionError`s.

- isolate a maybeGossipSystemConfig trigger

- remove all of the uses of `batch.Defer()` in `replica_command.go`, for
  instance on Split, Merge (required some stats cleanup), GC, TruncateLog,
  ChangeFrozen, RequestLease, TransferLease; lift their side effects
  appropriately as well

- move call to `setDesc` to trigger

- remove Defer in handleRaftReady

- move checksum computation and verification to triggers

- reset ContainsEstimates in split post-commit trigger (in effect moving that
  hack up from splitTrigger)

- move lease success/error counter update to trigger
@tbg
Copy link
Member Author

tbg commented Oct 11, 2016

I declare this done. We've removed batch.Defer and *PostCommitTrigger is passed up from Raft commands. Some side effects might have snuck back in, but none that I know of.

@tbg tbg closed this as completed Oct 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tbg tbg

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@tbg