kvserver: use engine's filesystem #49717
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
|
This change is |
jbowens
commented
May 29, 2020
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/storage/pebble.go, line 967 at r1 (raw file):
if os.IsNotExist(err) { return nil }
I'll remove this once #49713 is merged and I rebase over master.
petermattis
reviewed
May 29, 2020
There was a problem hiding this comment.
Looks good overall. Glad to see this cleanup done!
Reviewable status:
pkg/kv/kvserver/replica_corruption.go, line 80 at r2 (raw file):
} func writeFile(fs fs.FS, path, contents string) error {
Should we add this convenience function to the storage/fs package?
pkg/storage/pebble.go, line 477 at r2 (raw file):
var auxDir string if cfg.Dir == "" { // TODO(peter): This is horribly hacky but matches what RocksDB does. For
Yay!
pkg/storage/pebble.go, line 978 at r2 (raw file):
// this because they are accidentally creating the required directory on the // actual filesystem instead of in the memory filesystem. See // diskSideloadedStorage and SSTSnapshotStrategy.
I wonder if this TODO can be addressed now.
pkg/storage/rocksdb.go, line 3590 at r2 (raw file):
// The RocksDB Env doesn't expose a Stat equivalent. If we're using an // on-disk filesystem, circumvent the Env and return the os.Stat results. // The file sizes of encrypted files might be off.
I don't think this last sentence is true. The file sizes of encrypted files will be exactly the on-disk size. Am I missing something?
pkg/storage/tee.go, line 594 at r2 (raw file):
return nil, err } // TODO(jackson): compare the FileInfos?
Yeah. Maybe. Can we assert the files are the same size?
jbowens
added a commit
to jbowens/cockroach
that referenced
this pull request
Jun 1, 2020
4887c526 vfs: return nil from MemFS.RemoveAll if parent doesn't exist Prerequistite for cockroachdb#49717. Release note: None
jbowens
added a commit
to jbowens/cockroach
that referenced
this pull request
Jun 1, 2020
4887c526 vfs: return nil from MemFS.RemoveAll if parent doesn't exist Prerequistite for cockroachdb#49717. Release note: None
jbowens
force-pushed
the
jackson/kvfs
branch
5 times, most recently
from
df3ea21
to
b71b7e0
Compare
Update filesystem access to always go through the storage engine's filesystem interface, which ensures correctness for in-mem and encrypted filesystems Also, add a Stat function to the `storage/fs.FS` interface. The RocksDB implementation is still a hack, because the RocksDB Env doesn't expose sufficient information for implementing. For on-disk RocksDB engines, this implementation circumvents the Env, performing a direct os.Stat of the filesystem. For in-memory RocksDB engines, it provides a mocked `os.FileInfo` implementation that panics when any of its methods are invoked. This is sufficient for existing call sites that use Stat to check for existence. Fixes cockroachdb#42034. Related to cockroachdb#31913. Release note: None
jbowens
commented
Jun 5, 2020
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/replica_corruption.go, line 80 at r2 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Should we add this convenience function to the
storage/fspackage?
Done.
pkg/storage/pebble.go, line 978 at r2 (raw file):
Previously, petermattis (Peter Mattis) wrote…
I wonder if this TODO can be addressed now.
Done.
pkg/storage/rocksdb.go, line 3590 at r2 (raw file):
Previously, petermattis (Peter Mattis) wrote…
I don't think this last sentence is true. The file sizes of encrypted files will be exactly the on-disk size. Am I missing something?
Yeah, you're right. Done.
pkg/storage/tee.go, line 594 at r2 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Yeah. Maybe. Can we assert the files are the same size?
Done.
petermattis
approved these changes
Jun 5, 2020
|
bors r+ |
Build succeeded |
jbowens
added a commit
to jbowens/cockroach
that referenced
this pull request
Jun 15, 2020
Fix the TestSideloadingSideloadedStorage test when running with COCKROACH_STORAGE_ENGINE=rocksdb. The test has always depended on the exact error message surfaced from os.Remove. In cockroachdb#49717, diskSideloadStorage was modified to use the engine's RemoveDir operation rather than interacting with the filesystem directly. Since Pebble uses os.Remove for its implementation and emulates its error messages in its MemFS implementation, the exact message comparison continued to succeed. After cockroachdb#49717, the RocksDB env's error message was surfaced, with its own context prefixing. This updates the test to case-insensitively check for 'directory not empty' instead. Fixes cockroachdb#50226. Release note: None
craig bot
pushed a commit
that referenced
this pull request
Jun 15, 2020
49284: opt: synthesize check constraints on enum columns r=rohany a=rohany Fixes #49263. This PR teaches the optimizer how to synthesize check constraints on columns of an ENUM type, allowing queries like: ``` CREATE TYPE t AS ENUM ('howdy', 'hello'); CREATE TABLE tt (x t, y INT, PRIMARY KEY (x, y)); SELECT x, y FROM tt WHERE y = 2 ``` to be planned using constrained spans on the enum values, rather than a full table scan. Release note (performance improvement): Allow the optimizer to use enum information to generate better query plans. 50001: colexec: minor miscellaneous additions r=yuzefovich a=yuzefovich **colexec: add casts from datum-backed types to bools** While investigating unrelated test failures, I added this cast, so we might as well merge it. Addresses: #48135. Release note: None **colexec: add support for Values core with zero rows** Release note: None 50188: geo/geogfn: implement ST_Azimuth({geometry,geometry}) r=otan a=hueypark Fixes #48887 Release note (sql change): This PR implement adds the ST_Azimuth({geometry,geometry}) 50205: Makefile: ensure redact_safe.md generation is stable r=RichardJCai a=knz Fixes #50146 On some platforms, the default behavior of `git grep` is to number the lines (i.e. `-n` is implicitly added). This patch makes the `-n` explicit and tweaks the sed patterns to ensure that the behavior is stable. Release note: None 50239: kv/kvserver: disable the below-raft proto tracking in race builds r=petermattis a=petermattis The below-raft proto tracking is meant to catch bugs where we inadvertently starting marshaling a proto below Raft. This tracking is a source of signficant memory allocations which measurably slow down race builds. Since we're already doing this tracking in non-race builds, there is little benefit to also doing it in race builds. Disabling below-raft proto tracking for race builds reduces the time to run `testrace` on the `kv/kvserver` package from 605s to 517s on my laptop. Release note: None 50243: kv/kvserver: fix TestSideloadingSideloadedStorage w/ RocksDB r=jbowens a=jbowens Fix the TestSideloadingSideloadedStorage test when running with COCKROACH_STORAGE_ENGINE=rocksdb. The test has always depended on the exact error message surfaced from os.Remove. In #49717, diskSideloadStorage was modified to use the engine's RemoveDir operation rather than interacting with the filesystem directly. Since Pebble uses os.Remove for its implementation and emulates its error messages in its MemFS implementation, the exact message comparison continued to succeed. After #49717, when running with RocksDB, the RocksDB env's error message was surfaced, with its own context prefixing. This updates the test to case-insensitively check for 'directory not empty' instead. Fixes #50226. Release note: None Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: Jaewan Park <jaewan.huey.park@gmail.com> Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net> Co-authored-by: Peter Mattis <petermattis@gmail.com> Co-authored-by: Jackson Owens <jackson@cockroachlabs.com>
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.
Update filesystem access to always go through the storage engine's
filesystem interface, which ensures correctness for in-mem and encrypted
filesystems
Also, add a Stat function to the storage/fs.FS interface. The RocksDB
implementation is still a hack, because the RocksDB Env doesn't expose
sufficient information for implementing. For on-disk RocksDB engines,
this implementation circumvents the Env, performing a direct os.Stat
of the filesystem. For in-memory RocksDB engines, it provides a mocked
os.FileInfo implementation.
Fixes #42034.
Related to #31913.
Release note: None