roachtest,admission: assume ownership of adjacent tests#89027
Merged
craig[bot] merged 3 commits intocockroachdb:masterfrom Oct 3, 2022
Merged
roachtest,admission: assume ownership of adjacent tests#89027craig[bot] merged 3 commits intocockroachdb:masterfrom
craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
Member
|
This change is |
7878d25 to
8d82459
Compare
Release note: None
8d82459 to
8966a2c
Compare
Admission control has been enabled as default for multiple releases now, these roachtest variants that disable them aren't giving us value. If there are regressions observed in the standard kv/tpccbench tests that end up being attributable to admission control, we can learn about it when investigating the regression directly instead of needing to compare it to test runs where AC is disabled. Release note: None
We'll be adding lots of these tests and it's excessive to be running them nightly. These tests will also be reframed as benchmark tests with no explicit PASS/FAIL since we're not verifying cluster stability, but instead the degree to which latency/throughput is protected in the presence of resource saturation. There too a nightly cadence feels unnecessary to track regressions. Release note: None
sumeerbhola
approved these changes
Oct 3, 2022
Collaborator
sumeerbhola
left a comment
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @irfansharif)
pkg/cmd/roachtest/tests/admission_control_multi_store_overload.go line 26 at r1 (raw file):
) func registerMultiStoreOverload(r registry.Registry) {
I assumed this is just code movement so didn't look at the details.
irfansharif
commented
Oct 3, 2022
Contributor
Author
irfansharif
left a comment
There was a problem hiding this comment.
bors r+
Reviewable status:
pkg/cmd/roachtest/tests/admission_control_multi_store_overload.go line 26 at r1 (raw file):
Previously, sumeerbhola wrote…
I assumed this is just code movement so didn't look at the details.
It was.
Contributor
|
Build succeeded: |
craig bot
pushed a commit
that referenced
this pull request
Oct 5, 2022
89119: storage,kvserver: support lazy value fetching in SimpleMVCCIterator r=erikgrinaker a=sumeerbhola
SimpleMVCCIterator already separates the methods to retrieve the key and value, and callers who do not need any information about the value do not call UnsafeValue(). However, there are callers (like stats calculation) who need to know the length of the value and in some cases whether an MVCC value is a tombstone. The MVCCValueLenAndIsTombstone() and ValueLen() methods are introduced for these cases. This is preparation for the near future where retrieving the value in Pebble will have actual additional cost, since the value may be in a different part of the file or a different file.
Non-test callers that can use these new interfaces have been modified. There are some todos to modify some non-trivial callers, related to deciding what to GC, and checking sstable conflicts, which will happen in future PRs.
When new methods are introduced to retrieve these value attributes in pebble.Iterator, only pebbleIterator.{MVCCValueLenAndIsTombstone, ValueLen} will need to be modified to make use of them.
Release note: None
89191: roachtests: introduce admission-control/snapshot-overload r=irfansharif a=irfansharif
This test sets up a 3-node CRDB cluster on 8vCPU machines, loads it up
with a large TPC-C dataset, and sets up a foreground load of kv95/1b.
The TPC-C dataset has a replication of factor of 1 and is used as the
large lump we bus around through high-rate snapshots -- snapshots that
are send to the node containing leaseholders serving kv95 traffic.
Snapshots follow where the leases travel, cycling through each node,
evaluating performance isolation in the presence of snapshots.
Informs #80607.
This test comes 'battery-included', shipping with a custom grafana
dashboard and requisite prometheus/grafana plumbing. At the end of test
run a prometheus dump is kept around for later inspection. It also
integrates with --local and --skip-init for fast iteration. We're going
to cargo cult such things in future integration tests for admission
control.
Release note: None
---
There are a few other miscellaneous cleanups/conveniences around the roachtest package for `@cockroachdb/test-eng` to look at. First three commits are from #89027 and don't need to be looked at here.
89413: sql: test case granting create privilege for CREATE FUNCTION r=chengxiong-ruan a=chengxiong-ruan
Release note: None
Co-authored-by: sumeerbhola <sumeer@cockroachlabs.com>
Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
Co-authored-by: Chengxiong Ruan <chengxiongruan@gmail.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Release note: None