engine: propertly support batches containing only LogData #33122
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
Release note: None
|
This change is |
petermattis
approved these changes
Dec 13, 2018
There was a problem hiding this comment.
s/propertly/properly/g in the first commit message (which was admittedly my typo).
TestRocksDBWALFileEmptyBatch verifies that an empty batch does not result in a write to the log file. You could adapt that approach to verify that a batch containing only LogData does result in a write to the log file.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained
pkg/storage/engine/engine_test.go, line 798 at r2 (raw file):
// accidentally make Commit a no-op (via an errant fast-path) when a batch // contained only LogData. func TestWCommitBatchLogData(t *testing.T) {
s/TestWCommit/TestCommit/g
tbg
force-pushed
the
fix/engine-logdata-commit
branch
from
8f814be to
71397c0
Compare
tbg
commented
Dec 13, 2018
There was a problem hiding this comment.
Good idea, done. Wanna give it another glance? I verified it fails as expected when I undo your change to Empty().
Reviewable status:
pkg/storage/engine/engine_test.go, line 798 at r2 (raw file):
Previously, petermattis (Peter Mattis) wrote…
s/TestWCommit/TestCommit/g
Done.
petermattis
approved these changes
Dec 13, 2018
|
TFTR! bors r=petermattis |
Build failed |
Committing a batch that only contained LogData was accidentally turned into a no-op since the batch was erroneously considered empty. The change (in the preceding commit) makes cockroachdb#24591 not a no-op for the first time since that PR was merged. This bug was discovered during the following commit (see cockroachdb#32978) when the roachtest introduced therein would not detect a disk stall (assuming the logging partition wasn't also affected). Closes cockroachdb#32986. Release note: None
tbg
force-pushed
the
fix/engine-logdata-commit
branch
from
71397c0 to
f91c678
Compare
|
Fixed whitespace. bors r=petermattis |
craig bot
pushed a commit
that referenced
this pull request
Dec 14, 2018
33122: engine: propertly support batches containing only LogData r=petermattis a=tbg Release note: None Co-authored-by: Peter Mattis <[email protected]> Co-authored-by: Tobias Schottdorf <[email protected]>
Build succeeded |
irfansharif
added a commit
to irfansharif/cockroach
that referenced
this pull request
Oct 21, 2019
Previously a disk stall could allow a node to continue heartbeating its liveness record and prevent other nodes from taking over its leases, despite being completely unresponsive. This was first addressed in cockroachdb#24591 (+ cockroachdb#33122). This was undone in cockroachdb#32978 (which introduced a stricter version of a similar check). cockroachdb#32978 was later disabled by default in cockroachdb#36484, leaving us without the protections first introduced in cockroachdb#24591. This PR re-adds the logic from cockroachdb#24591. Release note: None.
craig bot
pushed a commit
that referenced
this pull request
Oct 21, 2019
41684: importccl: add 19.2 version gate check r=miretskiy a=miretskiy Ensure the cluster is fully upgraded when running import. Release note: ensure cluster fully upgraded when running import. 41711: storage/report: don't deserialize zone configs over and over r=andreimatei a=andreimatei This patch is expected to speedup reports generation considerably by not unmarshalling zone config protos for every range. Instead, the report-geneating visitors now keep state around the zone that a visited range is in and reuse that state if they're told that the following range is in the same zone. The range iteration infrastructure was enhanced to figure out when zones change from range to range and tell that to the visitors. Without this patch, generating the reports was pinning a core for minutes for a cluster with partitioning and 200k ranges. The profiles showed that it's all zone config unmarshalling. Fixes #41609 Release note (performance improvement): The performance of generating the system.replication_* reports was greatly improved for large clusters. 41761: storage: write to local storage before updating liveness r=bdarnell a=irfansharif Previously a disk stall could allow a node to continue heartbeating its liveness record and prevent other nodes from taking over its leases, despite being completely unresponsive. This was first addressed in #24591 (+ #33122). This was undone in #32978 (which introduced a stricter version of a similar check). #32978 was later disabled by default in #36484, leaving us without the protections first introduced in #24591. This PR re-adds the logic from #24591. Part of #41683. Release note: None. Co-authored-by: Yevgeniy Miretskiy <[email protected]> Co-authored-by: Andrei Matei <[email protected]> Co-authored-by: irfan sharif <[email protected]>
irfansharif
added a commit
to irfansharif/cockroach
that referenced
this pull request
Oct 21, 2019
Previously a disk stall could allow a node to continue heartbeating its liveness record and prevent other nodes from taking over its leases, despite being completely unresponsive. This was first addressed in cockroachdb#24591 (+ cockroachdb#33122). This was undone in cockroachdb#32978 (which introduced a stricter version of a similar check). cockroachdb#32978 was later disabled by default in cockroachdb#36484, leaving us without the protections first introduced in cockroachdb#24591. This PR re-adds the logic from cockroachdb#24591. Release note: None.
irfansharif
added a commit
to irfansharif/cockroach
that referenced
this pull request
Oct 21, 2019
Previously a disk stall could allow a node to continue heartbeating its liveness record and prevent other nodes from taking over its leases, despite being completely unresponsive. This was first addressed in cockroachdb#24591 (+ cockroachdb#33122). This was undone in cockroachdb#32978 (which introduced a stricter version of a similar check). cockroachdb#32978 was later disabled by default in cockroachdb#36484, leaving us without the protections first introduced in cockroachdb#24591. This PR re-adds the logic from cockroachdb#24591. Release note: None.
arulajmani
pushed a commit
to arulajmani/cockroach
that referenced
this pull request
Oct 29, 2019
Previously a disk stall could allow a node to continue heartbeating its liveness record and prevent other nodes from taking over its leases, despite being completely unresponsive. This was first addressed in cockroachdb#24591 (+ cockroachdb#33122). This was undone in cockroachdb#32978 (which introduced a stricter version of a similar check). cockroachdb#32978 was later disabled by default in cockroachdb#36484, leaving us without the protections first introduced in cockroachdb#24591. This PR re-adds the logic from cockroachdb#24591. 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.
Release note: None