-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
pkg/storage: enable EFOS, excise on snapshot, ingest splitting #116330
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Going to wait until cockroachdb/pebble#3147 makes its way up into Cockroach before landing this. |
ee7a715
to
9af9bc7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once the underlying bugfix lands
pkg/storage/pebble.go
Outdated
@@ -144,9 +144,9 @@ var UseExciseForSnapshots = settings.RegisterBoolSetting( | |||
var IngestSplitEnabled = settings.RegisterBoolSetting( | |||
settings.SystemOnly, | |||
"storage.ingest_split.enabled", | |||
"set to true to use ingest-time splitting to lower write-amplification (experimental)", | |||
"set to false to disable ingest-time splitting to lower write-amplification (experimental)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it currently sounds like disabling ingest-time splitting is the experimental option. We could probably reword this as "set to false to disable experimental ingest-time splitting that lowers write-amp", or drop the experimental altogether seeing as if it's defaulting to true, we should be able to stand behind it by the time 24.1 comes out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
b8ff822
to
705f454
Compare
cockroachdb/pebble@48b54c29 sstable: fix incorrect range key mask in virtualLast() Informs cockroachdb#116330. Release note: None. Epic: none
116633: server: dynamically increase compaction concurrency during downloads r=dt a=dt Early in an online restore, a workload may not be able to utilize all CPU due to the high latency of accessing still-remote data meaning that many or most queries spend most of their time waiting rather than executing. During this phase, increasing the compaciton concurrency can expedite getting data downloaded, to reduce that latency and improve the workload performance. However later in the download phase, when more of the most accessed data has been downloaded, the workload itself may be able to execute fast enough that it requires the majority of available CPU. At this point, excessive CPU usage by download compactions will actually negatively impact the workload performance. Thus it is desirable to maximize compaction concurrency when CPU is available but reduce it when becomes scarce. This change introduces an additional goroutine to the download call that monitors CPU usage and adjusts compaction concurrency up and down based on the CPU usage being below 70% or above 80% respectively. Release note: none. Epic: none. 116670: go.mod: bump Pebble to 48b54c29d8fe r=RaduBerinde a=itsbilal cockroachdb/pebble@48b54c29 sstable: fix incorrect range key mask in virtualLast() Informs #116330. Release note: None. Epic: none Co-authored-by: David Taylor <[email protected]> Co-authored-by: Bilal Akhtar <[email protected]>
The following three cluster settings were introduced in the 23.2 cycle, and are disabled by default in that release: - `storage.experimental.eventually_file_only_snapshots.enabled`, - `kv.snapshot_receiver.excise.enabled`, - `storage.ingest_split.enabled`. Enable all three by default, in preparation for 24.1. Fixes cockroachdb#115432. Release note: None.
705f454
to
4dd06b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Previously, we'd fall back to the 3 second consistency checker EventuallyFileOnlySnapshot (EFOS) wait in roachtests, which was slowing all of them down when we ran every replica through the consistency checker in post-test assertions. This change speeds up that consistency check in roachtest post-test assertions by flipping a new cluster setting to speed up EFOS waits for consistency checks after a roachtest finishes. Epic: none Unblocks cockroachdb#116330. Release note: None
#116892 will unblock the tests in this PR. |
Previously, we'd fall back to the 3 second consistency checker EventuallyFileOnlySnapshot (EFOS) wait in roachtests, which was slowing all of them down when we ran every replica through the consistency checker in post-test assertions. This change speeds up that consistency check in roachtest post-test assertions by flipping a new cluster setting to speed up EFOS waits for consistency checks after a roachtest finishes. Epic: none Unblocks cockroachdb#116330. Release note: None
116089: changefeedccl: support attributes in pubsub sink r=jayshrivastava a=jayshrivastava This change adds support for including the table name along with each row/batch sent by the v2 pubsub sink (enabled by default). The table name is passed inside pubsub attributes. Attributes are stored in a `map[string]string` and passed emitted alongside each with each message/batch. To enable this feature, the uri parameter `with_table_name_attribute=true` must be added to the sink uri. The key for the table name in the attribute map will be `TABLE_NAME`. Because this change needs to be backported, it is as small as possible to minimize risk. This feature can be expanded upon later to be more generic (ex. use changefeed options instead of env var, use cdc queries to specify custom attributes, use a generic metadata struct instead of tablename string, pass metadata to different sinks and not just pubsub etc). Because this feature will be expanded in the future, the release note is intentionally left blank. Release note: None Closes: #115426 116892: storage: add setting to seed up consistency checks in tests r=RaduBerinde a=itsbilal Previously, we'd fall back to the 3 second consistency checker EventuallyFileOnlySnapshot (EFOS) wait in roachtests, which was slowing all of them down when we ran every replica through the consistency checker in post-test assertions. This change speeds up that consistency check in roachtest post-test assertions by flipping a new cluster setting to speed up EFOS waits for consistency checks after a roachtest finishes. Epic: none Unblocks #116330. Release note: None Co-authored-by: Jayant Shrivastava <[email protected]> Co-authored-by: Bilal Akhtar <[email protected]>
Closing as #117116 has landed. |
The following three cluster settings were introduced in the 23.2 cycle, and are disabled by default in that release:
storage.experimental.eventually_file_only_snapshots.enabled
,kv.snapshot_receiver.excise.enabled
,storage.ingest_split.enabled
.Enable all three by default, in preparation for 24.1.
Fixes #115432.