storage/engine: centralize specification of pebble.Options #41901
Conversation
|
This change is |
|
Only look at the last commit. The first 2 commits are from #41839. |
petermattis
force-pushed
the
pmattis/pebble-options
branch
5 times, most recently
from
2cee9d7
to
7997e40
Compare
There was a problem hiding this comment.
Ping. This is a straightforward PR.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajkr, @itsbilal, and @sumeerbhola)
Fixes cockroachdb#41860 Release note: None
petermattis
force-pushed
the
pmattis/pebble-options
branch
from
7997e40
to
c3d602a
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/server/config.go, line 514 at r1 (raw file):
BlockSize: 32 << 10, }}, },
there are multiple configuration values we pass to RocksDB but not to Pebble -- ignoring the ExtraOptions and FileRegistry which are encryption related I see sizeInBytes, spec.Attributes, cfs.Settings. Why the difference?
There was a problem hiding this comment.
Reviewable status:
pkg/server/config.go, line 514 at r1 (raw file):
Previously, sumeerbhola wrote…
there are multiple configuration values we pass to RocksDB but not to Pebble -- ignoring the
ExtraOptionsandFileRegistrywhich are encryption related I seesizeInBytes,spec.Attributes,cfs.Settings. Why the difference?
ExtraOptions are encryption-at-rest options which you'll be implementing shortly. We do pass spec.Attributes and Settings (the latest commit is passing them in engine.StorageConfig. I'm not sure about sizeInBytes. Let me investigate.
There was a problem hiding this comment.
Reviewable status:
pkg/server/config.go, line 514 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
ExtraOptionsare encryption-at-rest options which you'll be implementing shortly. We do passspec.AttributesandSettings(the latest commit is passing them inengine.StorageConfig. I'm not sure aboutsizeInBytes. Let me investigate.
sizeInBytes seems to be an oversight. I've added another commit which plumbs it through, as well as does as small bit of cleanup for StorageConfig. PTAL.
There was a problem hiding this comment.
Reviewable status:
pkg/storage/engine/pebble.go, line 409 at r3 (raw file):
func (p *Pebble) Capacity() (roachpb.StoreCapacity, error) { // Pebble doesn't have a capacity limiting parameter, so pass 0 for // maxSizeBytes to denote no limit.
comment is stale
petermattis
force-pushed
the
pmattis/pebble-options
branch
from
f31ecd3
to
c0a9c1e
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/storage/engine/pebble.go, line 409 at r3 (raw file):
Previously, sumeerbhola wrote…
comment is stale
Thanks. Fixed.
Move `RocksDBConfig.MaxSizeBytes` to `StorageConfig.MaxSize` so it can be shared with Pebble. Embeded `StorageConfig` in both `RocksDBConfig` and `PebbleConfig` so simplify access to its fields. Release note: None
petermattis
force-pushed
the
pmattis/pebble-options
branch
from
c0a9c1e
to
25b8acf
Compare
|
TFTR! bors r+ |
41901: storage/engine: centralize specification of pebble.Options r=petermattis a=petermattis Fixes #41860 Release note: None 41993: build: Upgrade to go 1.12.12 r=bobvawter a=bobvawter This change upgrades the go runtime to 1.12.12 in order to pick up a [security fix](golang/go#34960). Per the [checklist](build/README.md): * [X] Adjust version in Docker image * [X] Rebuild the Docker image and bump the version in builder.sh accordingly * [ ] ~Bump the version in go-version-check.sh~ (Patch release, not necessary) * [X] Bump the default installed version of Go in bootstrap-debian.sh Fixes: #41718 Release note (build change): The go runtime has been upgraded to 1.12.12. Co-authored-by: Peter Mattis <[email protected]> Co-authored-by: Bob Vawter <[email protected]>
Build succeeded |
Fixes #41860
Release note: None