Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kvserver: Switch to using EventuallyFileOnlySnapshots for consistency checker and mvcc GC #106775

Closed
itsbilal opened this issue Jul 13, 2023 · 2 comments · Fixed by #109276
Closed
Assignees
Labels
A-storage Relating to our storage engine (Pebble) on-disk storage. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-storage Storage Team

Comments

@itsbilal
Copy link
Contributor

itsbilal commented Jul 13, 2023

Currently, the replica consistency checker uses a long lived Pebble snapshot for checking consistency between replicas. It grabs this snapshot at the same index across all replicas, with some coordination over Raft. The MVCC GC queue also uses a pebble snapshot.

However, since IngestAndExcise as well as #105839 break the guarantees traditionally provided by snapshots in Pebble, we need to get users of Pebble snapshots onto other abstractions with similar guarantees. A long-lived iterator is one option, however consistency checking can be really slow and long-lived so the behaviour of iterators around pinning memtables is really not desirable here. An EventuallyFileOnlySnapshot would serve the consistency checker and MVCC GC queue very well.

This change asks for a wholesale move of the consistency checker and MVCC GC queue to EventuallyFileOnlySnapshot to allow for broader use of IngestAndExcise in reducing write-amp in Pebble.

Dependency (and more context) in cockroachdb/pebble#2740

Jira issue: CRDB-29712

@itsbilal itsbilal added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Jul 13, 2023
@exalate-issue-sync exalate-issue-sync bot added the T-storage Storage Team label Aug 8, 2023
@blathers-crl blathers-crl bot added the A-storage Relating to our storage engine (Pebble) on-disk storage. label Aug 8, 2023
@exalate-issue-sync exalate-issue-sync bot removed T-storage Storage Team A-storage Relating to our storage engine (Pebble) on-disk storage. labels Aug 9, 2023
@jbowens jbowens added A-storage Relating to our storage engine (Pebble) on-disk storage. T-storage Storage Team labels Aug 10, 2023
@jbowens
Copy link
Collaborator

jbowens commented Aug 10, 2023

Is there already an issue for switching MVCC GC over?

@itsbilal itsbilal changed the title kvserver: Switch to using EventuallyFileOnlySnapshots for consistency checker kvserver: Switch to using EventuallyFileOnlySnapshots for consistency checker and mvcc GC Aug 14, 2023
@itsbilal
Copy link
Contributor Author

@jbowens I updated this issue to also cover the MVCC GC case. Both should be easy to handle at the same time.

itsbilal added a commit to itsbilal/cockroach that referenced this issue Aug 22, 2023
This change migrates over some uses of pebble Snapshots to
EventuallyFileOnlySnapshots. The latter incur no write-amp
cost unlike classic Snapshots, with the primary tradeoff
being that they hold onto sstables like an iterator does,
so there's a space-amp cost instead.

In the shorter term, an EventuallyFileOnlySnapshot (EFOS)
also pins memtables in memory, but eventually moves away
from them onto just pinning sstables in storage.

Since the replica consistency checker can run for longer
periods of time with rate-limiting, it waits for a transition
to a file-only snapshot before proceeding with a request. Other
uses of EFOS use it as a drop-in replacement for a snapshot.

Fixes cockroachdb#106775.

Epic: none

Release note: None
itsbilal added a commit to itsbilal/cockroach that referenced this issue Sep 5, 2023
This change migrates over some uses of pebble Snapshots to
EventuallyFileOnlySnapshots. The latter incur no write-amp
cost unlike classic Snapshots, with the primary tradeoff
being that they hold onto sstables like an iterator does,
so there's a space-amp cost instead.

In the shorter term, an EventuallyFileOnlySnapshot (EFOS)
also pins memtables in memory, but eventually moves away
from them onto just pinning sstables in storage.

Since the replica consistency checker can run for longer
periods of time with rate-limiting, it waits for a transition
to a file-only snapshot before proceeding with a request. Other
uses of EFOS use it as a drop-in replacement for a snapshot.

Fixes cockroachdb#106775.

Epic: none

Release note: None
itsbilal added a commit to itsbilal/cockroach that referenced this issue Sep 9, 2023
This change migrates over some uses of pebble Snapshots to
EventuallyFileOnlySnapshots. The latter incur no write-amp
cost unlike classic Snapshots, with the primary tradeoff
being that they hold onto sstables like an iterator does,
so there's a space-amp cost instead.

In the shorter term, an EventuallyFileOnlySnapshot (EFOS)
also pins memtables in memory, but eventually moves away
from them onto just pinning sstables in storage.

Since the replica consistency checker can run for longer
periods of time with rate-limiting, it waits for a transition
to a file-only snapshot before proceeding with a request. Other
uses of EFOS use it as a drop-in replacement for a snapshot.

Fixes cockroachdb#106775.

Epic: none

Release note: None
@itsbilal itsbilal self-assigned this Sep 11, 2023
craig bot pushed a commit that referenced this issue Sep 11, 2023
109276: storage, kvserver: Move uses of snapshots to EventuallyFileOnlySnapshot r=sumeerbhola a=itsbilal

This change migrates over some uses of pebble Snapshots to EventuallyFileOnlySnapshots. The latter incur no write-amp cost unlike classic Snapshots, with the primary tradeoff being that they hold onto sstables like an iterator does, so there's a space-amp cost instead.

In the shorter term, an EventuallyFileOnlySnapshot (EFOS) also pins memtables in memory, but eventually moves away from them onto just pinning sstables in storage.

Since the replica consistency checker can run for longer periods of time with rate-limiting, it waits for a transition to a file-only snapshot before proceeding with a request. The same is with MVCC GC. Other uses of EFOS use it as a drop-in replacement for a snapshot if the `storage.experimental.use-eventually-file-only-snapshots` cluster setting is enabled or if shared storage is in use.

Fixes #106775.
Fixes #109455

Epic: none

Release note: None

110353: kvserver: deflake TestStoreMetrics r=jbowens a=jbowens

Resolve a test flake in TestStoreMetrics by reverting to stopping the TestServers in order to stabilize metrics. To recompute the metrics, the store's engine is reopened in read-only mode directly and closed again before restarting servers.

Epic: none
Fixes #109240.
Release note: None

110387: dev: don't "stress" `disallowed_imports_test` r=healthy-pod a=rickystewart

These tests cannot be flaky and don't benefit from stressing.

Closes #110351.

Epic: CRDB-17171
Release note: None

Co-authored-by: Bilal Akhtar <[email protected]>
Co-authored-by: Jackson Owens <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
@craig craig bot closed this as completed in 22742c6 Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-storage Relating to our storage engine (Pebble) on-disk storage. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-storage Storage Team
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants