raft: fix Ready.MustSync logic #10106
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
lgtm. The commit title needs to be changed to "raft: fix ..." |
benesch
added a commit
to benesch/cockroach
that referenced
this pull request
Sep 19, 2018
Committing an empty batch previously wrote an empty entry to the RocksDB WAL file. Coupled with a bug in Raft (etcd-io/etcd#10106), this was causing unnecessary synchronous writes to disk during Raft heartbeats. A non-rigorous benchmark shows that this yields a nearly 10% performance improvement on a three node cluster under a write-heavy workload (kv5%). Release note: None
petermattis
force-pushed
the
pmattis/ready-must-sync
branch
from
3696316
to
6b8b171
Compare
Done. Thanks for the quick review. |
benesch
added a commit
to benesch/cockroach
that referenced
this pull request
Sep 19, 2018
Committing an empty batch previously wrote an empty entry to the RocksDB WAL file. Coupled with a bug in Raft (etcd-io/etcd#10106), this was causing unnecessary synchronous writes to disk during Raft heartbeats. A non-rigorous benchmark shows that this yields a nearly 10% performance improvement on a three node cluster under a write-heavy workload (kv5%). Release note: None
craig bot
pushed a commit
to cockroachdb/cockroach
that referenced
this pull request
Sep 19, 2018
30398: engine: avoid committing empty batches r=nvanbenschoten,petermattis,bdarnell a=benesch Committing an empty batch previously wrote an empty entry to the RocksDB WAL file. Coupled with a bug in Raft (etcd-io/etcd#10106), this was causing unnecessary synchronous writes to disk during Raft heartbeats. A non-rigorous benchmark shows that this yields a nearly 10% performance improvement on a three node cluster under a write-heavy workload (kv5%). ^ @nvanbenschoten can this be right??? I don't trust my numbers, but I don't have time to run another benchmark tonight. Note that the explosion of C++ is just to get at the RocksDB WAL's file size in a unit test. It's not a code path exercised anywhere but tests. Release note: None Co-authored-by: Nikhil Benesch <nikhil.benesch@gmail.com>
|
CI failed with |
The previous logic was erroneously setting Ready.MustSync to true when the hard state had not changed because we were comparing an empty hard state to the previous hard state. In combination with another misfeature in CockroachDB (unnecessary writing of empty batches), this was causing a steady stream of synchronous writes to disk.
petermattis
force-pushed
the
pmattis/ready-must-sync
branch
from
6b8b171
to
66ee394
Compare
|
Fixed the shadow warning. |
petermattis
added a commit
to cockroachdb/vendored
that referenced
this pull request
Sep 19, 2018
This picks up etcd-io/etcd#10106.
petermattis
added a commit
to cockroachdb/vendored
that referenced
this pull request
Sep 19, 2018
This picks up etcd-io/etcd#10106.
petermattis
added a commit
to cockroachdb/vendored
that referenced
this pull request
Sep 20, 2018
This picks up etcd-io/etcd#10106.
petermattis
added a commit
to petermattis/cockroach
that referenced
this pull request
Sep 20, 2018
This picks up etcd-io/etcd#10106 which fixes an issue where `raft.Ready.MustSync` was being set too frequently resulting in unnecessary synchronous writes to the RocksDB WAL. Release note (performance improvement): Remove unnecessary synchronous disk writes caused by erroneous logic in the Raft implementation.
craig bot
pushed a commit
to cockroachdb/cockroach
that referenced
this pull request
Sep 20, 2018
30443: vendor: bump go.etcd.io/etcd r=nvanbenschoten,benesch a=petermattis This picks up etcd-io/etcd#10106 which fixes an issue where `raft.Ready.MustSync` was being set too frequently resulting in unnecessary synchronous writes to the RocksDB WAL. Release note (performance improvement): Remove unnecessary synchronous disk writes caused by erroneous logic in the Raft implementation. Co-authored-by: Peter Mattis <petermattis@gmail.com>
petermattis
added a commit
to petermattis/cockroach
that referenced
this pull request
Sep 20, 2018
This picks up etcd-io/etcd#10106 which fixes an issue where `raft.Ready.MustSync` was being set too frequently resulting in unnecessary synchronous writes to the RocksDB WAL. Release note (performance improvement): Remove unnecessary synchronous disk writes caused by erroneous logic in the Raft implementation.
petermattis
added a commit
to cockroachdb/vendored
that referenced
this pull request
Sep 20, 2018
This picks up etcd-io/etcd#10106.
benesch
added a commit
to benesch/cockroach
that referenced
this pull request
Sep 24, 2018
Committing an empty batch previously wrote an empty entry to the RocksDB WAL file. Coupled with a bug in Raft (etcd-io/etcd#10106), this was causing unnecessary synchronous writes to disk during Raft heartbeats. A non-rigorous benchmark shows that this yields a nearly 10% performance improvement on a three node cluster under a write-heavy workload (kv5%). Release note: None
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The previous logic was erroneously setting Ready.MustSync to true when
the hard state had not changed because we were comparing an empty hard
state to the previous hard state. In combination with another misfeature
in CockroachDB (unnecessary writing of empty batches), this was causing
a steady stream of synchronous writes to disk.