bazel: make dev bench disable crdb_test #86167
Merged
Conversation
This file contains 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
|
This change is |
irfansharif
approved these changes
Aug 15, 2022
pkg/cmd/dev/bench.go
Outdated
| benchMemFlag = "bench-mem" | ||
| benchTimeFlag = "bench-time" | ||
| benchMemFlag = "bench-mem" | ||
| crdbTestOnFlag = "crdb-test-on" |
There was a problem hiding this comment.
Drop the "-on", this is already a boolean flag so the "on" feels implied. I would actually prefer not providing this as a flag in dev at all since it's unlikely that it should ever be used (make didn't), and if it is, you can through dev bench -- --crdb_test --test_env=COCKROACH_INTERNAL_DISABLE_METAMORPHIC_TESTING=true. If folks find a need for it, or notice a regression, we can add this flag then.
There was a problem hiding this comment.
So if bazel sees --crdb_test_off followed by --crdb_test last one wins? Cool, I'll remove it.
cucaroach
force-pushed
the
bazel-bench-crdb-test-off
branch
from
c73d3e9 to
87cbebd
Compare
|
Hit a flake: #86215 |
cucaroach
force-pushed
the
bazel-bench-crdb-test-off
branch
from
87cbebd to
97aec0d
Compare
|
bors r+ |
|
👎 Rejected by PR status |
|
bors r+ |
|
👎 Rejected by PR status |
crdb_test enables metamorphic variables and various "extra" debug checking code paths that can interfere with performance testing. This is also consistent with how make bench behaved. If for whatever reason crdb_test is desirable just pass --crdb_test to the underlying bazel invocation. If you want crdb_test on but no metamorphic variables the bazel test_env argument can be used like so: --test_env=COCKROACH_INTERNAL_DISABLE_METAMORPHIC_TESTING=true Informs: cockroachdb#83599 Release note: None Release justification: non-production code change
|
Just added a release justification. bors r+ |
|
Build failed: |
|
bors r+ |
cucaroach
force-pushed
the
bazel-bench-crdb-test-off
branch
from
97aec0d to
220d670
Compare
|
Canceled. |
|
bors r+ |
|
Build succeeded: |
yuzefovich
added a commit
to yuzefovich/cockroach
that referenced
this pull request
Aug 26, 2022
This commit bumps version since there appears to have been a "merge skew" between cockroachdb#85095 and cockroachdb#86167, and somehow I had a `dev` binary that didn't include the benchmark fix from the latter. Release justification: test-only change. Release note: None
Merged
craig bot
pushed a commit
that referenced
this pull request
Aug 26, 2022
83531: rfc: add rfc for invisible index feature r=wenyihu6 a=wenyihu6 This commit adds an RFC for the invisible index feature. Related issue: #72576, #82363 Release justification: low risk to the existing functionality; this commit just adds rfc. Release Note: none 86267: allocator: select a good enough store for decom/recovery r=lidorcarmel a=lidorcarmel Until now, when decommissioning a node, or when recovering from a dead node, the allocator tries to pick one of the best possible stores as the target for the recovery. Because of that, we sometimes see multiple stores recover replicas to the same store, for example, when decommissioning a node and at the same time adding a new node. This PR changes the way we select a destination store by choosing a random store out of all the stores that are "good enough" for the replica. The risk diversity is still enforced, but we may recover a replica to a store that is considered "over full", for example. Note that during upreplication the allocator will still try to use one of the "best" stores as targets. Fixes: #86265 Release note: None Release justification: a relatively small change, and it can be reverted by setting kv.allocator.recovery_store_selector=best. 86345: clusterversion: prevent upgrades from development versions r=ajwerner a=dt This change defines a new "unstableVersionsAbove" point on the cluster version line, above which any cluster versions are considered unstable development-only versions which are still subject to change. Performing an upgrade to a version while it is still unstable leaves a cluster in a state where it persists a version that claims it has done that upgrade and all prior, however those upgrades are still subject to change by nature of being unstable. If it subsequently upgraded to a stable version, this could result in subtle and nearly impossible to detect issues, as being at or above a particular version is used to assume that all subsequent version upgrades _as released_ were run; on a cluster that ran an earlier iteration of an upgrade this does not hold. Thus to prevent clusters which upgrade to development versions from subsequently upgrading to a stable version, we offset all development versions -- those above the unstableVersionsAbove point -- into the far future by adding one million to their major version e.g. v22.x-y becomes 1000022.x-y. This means an attempt to subsequently "upgrade" to a stable version -- such as v22.2 -- will look like a downgrade and be forbidden. On the release branch, prior to starting to publish upgradable releases, the unstableVersionsAbove value should be set to invalidVersionKey to reflect that all version upgrades in that release branch are now considered to be stable, meaning they must be treated as immutable and append-only. Release note (ops change): clusters that are upgraded to an alpha or other manual build from the development branch will not be able to be subsequently upgraded to a release build. Release justification: high-priority change to existing functionality, to allow releasing alphas with known version upgrade bugs while ensuring they do not subsequently upgrade into stable version but silently corrupted clusters. 86630: kvserver: add additional testing to multiqueue r=AlexTalks a=AlexTalks Add testing for cancelation of multi-queue requests and fix a bug where the channel wasn't closed on task cancelation. Release justification: Test-only change. Release note: None 86801: ttl: add queries to job details r=otan a=rafiss fixes #81905 This helps with observability so users can understand what the TTL job is doing behind the scenes. The job details in the DB console will show: ``` ttl for defaultdb.public.t -- for each range, iterate to find rows: SELECT id FROM [108 AS tbl_name] AS OF SYSTEM TIME '30s' WHERE <crdb_internal_expiration OR ttl_expiration_expression> <= $1 AND (id) > (<range start>) AND (id) < (<range end>) ORDER BY id LIMIT <ttl_select_batch_size> -- then delete with: DELETE FROM [108 AS tbl_name] WHERE <crdb_internal_expiration OR ttl_expiration_expression> <= $1 AND (id) IN (<rows selected above>) ``` Release note: None Release justification: low risk change 86876: kv: Include error information in `crdb_internal.active_range_feeds` r=miretskiy a=miretskiy Include error count, and the last error information in `crdb_internal.active_range_feeds` table whenever rangefeed disconnects due to an error. Release justification: observability improvement. Release note: None 86901: sql: fix cluster_execution_insights priority level r=j82w a=j82w This fixes the priority level and converts it to be a string. closes: #86900, closes #86867 Release justification: Category 2: Bug fixes and low-risk updates to new functionality Release note (sql change): Fix the insight execution priority to have correct value instead of always being default. Changed the column to be a string to avoid converting it in the ui. 86921: kvserver: incorporate sending queue priority into snapshot requests r=AlexTalks a=AlexTalks This change modifies the `(Delegated)SnapshotRequest` Raft RPCs in order to incorporate the name of the sending queue, as well as the sending queue's priority, in order to be used to prioritize queued snapshots on a receiving store. Release justification: Low-risk change to existing functionality. Release note: None 86923: dev: bump version r=yuzefovich a=yuzefovich This commit bumps version since there appears to have been a "merge skew" between #85095 and #86167, and somehow I had a `dev` binary that didn't include the benchmark fix from the latter. Release justification: test-only change. Release note: None Co-authored-by: wenyihu3 <[email protected]> Co-authored-by: Lidor Carmel <[email protected]> Co-authored-by: David Taylor <[email protected]> Co-authored-by: Alex Sarkesian <[email protected]> Co-authored-by: Rafi Shamim <[email protected]> Co-authored-by: Yevgeniy Miretskiy <[email protected]> Co-authored-by: j82w <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
crdb_test enables metamorphic variables and various "extra" debug
checking code paths that can interfere with performance testing. This
is also consistent with how make bench behaved.
If you want crdb_test on but no metamorphic variables the bazel test_env
argument can be used like so:
--test_env=COCKROACH_INTERNAL_DISABLE_METAMORPHIC_TESTING=true
Informs: #83599
Release note: None
Release justification: Non-production code