Commits on Sep 23, 2017

1. [DNM] storage: respond to Raft proposals after entries commit, not ex… …

   …ecute
   
   This change addresses the first optimization discussed in cockroachdb#17500.
   
   The change seems to work and provides a modest performance boost.
   Unfortunately, I don't think we'll want to consider merging it at
   the moment. The problem is that while it is technically safe to
   respond to clients before performing the Raft command application,
   doing so is a nightmare for testing. Pretty much every
   test in the `storage` package expects to be able to perform an
   operation and then "reach beneath raft" immediately to operate
   on the result. This can range from inspecting Raft entries to
   working on the most up-to-date `Replica` state.
   
   To support this change, all of these tests would need to be
   updated to handle the now asynchronous operations performed
   in `handleEvalResultRaftMuLocked`. I addressed this by adding
   a testing knob called `DisableRaftRespBeforeApplication` in
   this change. The problem is that I don't feel very comfortable
   with it because we basically need to use it for all tests
   (indirectly through `multiTestContext` and `LocalTestCluster`)
   which means that we probably aren't testing this optimization
   thoroughly. We could disable the optimization on a finer
   granularity but this would become a serious issue for
   maintainability and I'm not sure it would be worth it.
   
   Perhaps there's some middle ground between returning to the
   client after performing in-memory state updates but before
   performing persistent state updates? Something like calling:
   1. `handleEvalResultRaftMuLocked`
   2. `maybeRespondToClient`
   3. `applyRaftCommand`
   
   This would solve a lot of the testing issues present here without
   the need to use the `DisableRaftRespBeforeApplication` knob, but
   I'm almost certain that wouldn't be safe to do.
   
   I think cockroachdb#15648 will run into a similar issue to this. We'll either
   need to block clients while we combine Raft batches or we'll need
   to update tests which expect a client response to be an indication
   that the command has already been applied in all cases. Things
   might not be as bad in that case though because less is being
   done asynchronously.

   

   nvanbenschoten committed Sep 23, 2017
   Configuration menu

   • View commit details
   • Copy full SHA for 79ddf9d
   • Browse repository at this point

   Copy the full SHA

   79ddf9d View commit details

   Browse the repository at this point in the history