-
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
release-23.2: storage: add cluster setting for ingest splits #115642
Conversation
5c88f44
to
1ce92f5
Compare
Thanks for opening a backport. Please check the backport criteria before merging:
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
Also, please add a brief release justification to the body of your PR to justify this |
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. |
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @blathers-crl[bot] and @RaduBerinde)
pkg/storage/pebble.go
line 140 at r1 (raw file):
"set to true to use ingest-time splitting to lower write-amplification (experimental)", util.ConstantWithMetamorphicTestBool( "storage.ingest_split.enabled", false), /* defaultValue */
I am wary of enabling this metamorphically on the release-23.2 branch before there's been any baking on master, especially given we're not yet at a release candidate.
+1 to @jbowens's comment. Perhaps we enable metamorphically on Otherwise EM LGTM ✅ |
Previously, the ingest-time splitting feature was only tunable through a code change. This change adds a cluster setting to allow this feature to be turned on dynamically. Fixes #115430. Epic: none Release note: None Release justification: low-risk addition of a cluster setting to be able to toggle a new, experimental feature.
1ce92f5
to
6091349
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.
TFTR!
We can roll in the default value true
change into #115432 probably? Should be easy to have one PR that just changes the defaults to true for three cluster settings.
Reviewed 2 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jbowens and @RaduBerinde)
pkg/storage/pebble.go
line 140 at r1 (raw file):
Previously, jbowens (Jackson Owens) wrote…
I am wary of enabling this metamorphically on the release-23.2 branch before there's been any baking on master, especially given we're not yet at a release candidate.
Done.
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
Backport 1/1 commits from #115462 on behalf of @itsbilal.
/cc @cockroachdb/release
Previously, the ingest-time splitting feature was only tunable through a code change. This change adds a cluster setting to allow this feature to be turned on dynamically.
Fixes #115430.
Epic: none
Release note: None
Release justification: low-risk addition of a cluster setting to be able to toggle a new, experimental feature.