kvserver: revamp raft proto test #93785

tbg · 2022-12-16T13:34:58Z

We previously had this setup where we put an interceptor into
protoutil.Marshal, ran all of the kvserver tests (or actually just
whichever ones were invoked... since you could run with filters or just
be a bazel shard), and collected the stacks seen for protoutil.Marshal
and the message types for which we saw them.

We then attempted to make sure that the set of protos that was marshaled
below raft (though the definition of "below raft" was fuzzy; the code
was really only looking at command application) was special in that
any changes to the marshaling would cause a test failure that would
have to be "signed off on" separately by updating the fixtures.

This was all too clever, and too magical. It also had a real downside:
by injecting its magic the package tests, it also slowed them down, and
in particular it would mess with benchmark results: reflection and extra
allocations can really skew the picture (which is what prompted this
commit).

This commit rips out the existing meta test and replaces it with a
vanilla Go test that keeps an explicit list of protos that we alert on
should their encoding change. Thanks to echotest, it's now easy to
regenerate the fixtures if needed.

Epic: none
Release note: None

cockroach-teamcity · 2022-12-16T13:35:06Z

This change is

We previously had this setup where we put an interceptor into `protoutil.Marshal`, ran all of the `kvserver` tests (or actually just whichever ones were invoked... since you could run with filters or just be a bazel shard), and collected the stacks seen for `protoutil.Marshal` and the message types for which we saw them. We then attempted to make sure that the set of protos that was marshaled below raft (though the definition of "below raft" was fuzzy; the code was really only looking at command application) was special in that any changes to the marshaling would cause a test failure that would have to be "signed off on" separately by updating the fixtures. This was all too clever, and too magical. It also had a real downside: by injecting its magic the package tests, it also slowed them down, and in particular it would mess with benchmark results: reflection and extra allocations can really skew the picture (which is what prompted this commit). This commit rips out the existing meta test and replaces it with a vanilla Go test that keeps an explicit list of protos that we alert on should their encoding change. Thanks to `echotest`, it's now easy to regenerate the fixtures if needed. Epic: none Release note: None

erikgrinaker

Thanks, I like the simplification here.

pkg/kv/kvserver/below_raft_protos_test.go

tbg · 2022-12-16T15:50:26Z

bors r=erikgrinaker

craig · 2022-12-16T16:25:30Z

Build failed:

Bazel Essential CI (Cockroach)

tbg · 2022-12-16T20:19:20Z

bors r=erikgrinaker

Flaked on TestProtectedTimestamps, notified flaky test fighters

craig · 2022-12-16T21:52:44Z

Build succeeded:

Bazel Essential CI (Cockroach)

Reproposals are a deep rabbit hole and an area in which past changes were all related to subtle bugs. Write it all up and in particular make some simplifications that ought to be possible if my understanding is correct: - have proposals always enter `(*Replica).propose` without a MaxLeaseIndex or prior encoded command set, i.e. `propose` behaves the same for reproposals as for first proposals. - assert that after a failed call to tryReproposeWithNewLeaseIndex, the command is not in the proposals map, i.e. check absence of a leak. - replace code that should be impossible to reach (and had me confused for a good amount of time) with an assertion. - add long comment on `r.mu.proposals`. This commit also moves `tryReproposeWithNewLeaseIndex` off `(*Replica)`, which is possible due to recent changes[^1]. In doing so, I realized there was a (small) data race (now fixed): when returning a `NotLeaseholderError` from that method, we weren't acquiring `r.mu`. It may have looked as though we were holding it already since we're accessing `r.mu.propBuf`, however that field has special semantics - it wraps `r.mu` and acquires it when needed. [^1]: The "below raft" test mentioned in the previous comment was changed in cockroachdb#93785 and no longer causes a false positive. Epic: CRDB-220 Release note: None

89752: jobs/cdc: add metrics for paused jobs r=miretskiy a=jayshrivastava This change adds new metrics to count paused jobs for every job type. For example, the metric for paused changefeed jobs is `jobs.changefeed.currently_paused`. These metrics are counted at an interval defined by the cluster setting `jobs.metrics.interval.poll`. This is implemented by a job which periodically queries `system.jobs` to count the number of paused jobs. This job is of the newly added type `jobspb.TypePollJobsStats`. When a node starts it's job registry, it will create an adoptable stats polling job if it does not exist already using a transaction. This change adds a test which pauses and resumes changefeeds while asserting the value of the `jobs.changefeed.currently_paused` metric. It also adds a logictest to ensure one instance of the stats polling job is created in a cluster. Resolves: #85467 Release note (general change): This change adds new metrics to count paused jobs for every job type. For example, the metric for paused changefeed jobs is `jobs.changefeed.currently_paused`. These metrics are updated at an interval defined by the cluster setting `jobs.metrics.interval.poll`, which is defauled to 10 seconds. Epic: None 94633: kvserver: document reproposals r=nvanbenschoten a=tbg Reproposals are a deep rabbit hole and an area in which past changes were all related to subtle bugs. Write it all up and in particular make some simplifications that ought to be possible if my understanding is correct: - have proposals always enter `(*Replica).propose` without a MaxLeaseIndex or prior encoded command set, i.e. `propose` behaves the same for reproposals as for first proposals. - assert that after a failed call to tryReproposeWithNewLeaseIndex, the command is not in the proposals map, i.e. check absence of a leak. - replace code that should be impossible to reach (and had me confused for a good amount of time) with an assertion. - add long comment on `r.mu.proposals`. This commit also moves `tryReproposeWithNewLeaseIndex` off `(*Replica)`, which is possible due to recent changes[^1]. In doing so, I realized there was a (small) data race (now fixed): when returning a `NotLeaseholderError` from that method, we weren't acquiring `r.mu`. It may have looked as though we were holding it already since we're accessing `r.mu.propBuf`, however that field has special semantics - it wraps `r.mu` and acquires it when needed. [^1]: The "below raft" test mentioned in the previous comment was changed in #93785 and no longer causes a false positive. Epic: CRDB-220 Release note: None 96650: kvserver: extract kvstorage.DestroyReplica r=pavelkalinnikov a=tbg This series of commits extracts `(*Replica).preDestroyRaftMuLocked` into a free-standing method `kvstorage.DestroyReplica`. In doing so, it separates the in-memory and on-disk steps that are a part of replica removal, and makes the on-disk steps unit testable. Touches #93241. Epic: CRDB-220 Release note: None 96659: sql: wrap stacktraceless errors with errors.Wrap r=andreimatei a=ecwall Fixes #95794 This replaces the previous attempt to add logging here #95797. The context itself cannot be augmented to add a stack trace to errors because it interferes with grpc timeout logic - gRPC compares errors directly without checking causes https://github.com/grpc/grpc-go/blob/v1.46.0/rpc_util.go#L833. Although the method signature allows it, `Context.Err()` should not be overriden to customize the error: ``` // If Done is not yet closed, Err returns nil. // If Done is closed, Err returns a non-nil error explaining why: // Canceled if the context was canceled // or DeadlineExceeded if the context's deadline passed. // After Err returns a non-nil error, successive calls to Err return the same error. Err() error ``` Additionally, a child context of the augmented context may end up being used which will circumvent the stack trace capture. This change instead wraps `errors.Wrap` in a few places that might end up helping debug the original problem: 1) Where we call `Context.Err()` directly. 2) Where gRPC returns an error after possibly calling `Context.Err()` internally or returns an error that does not have a stack trace. Release note: None 96770: storage: don't modify the given cfg.Opts r=RaduBerinde a=RaduBerinde This change improves the `NewPebble` code to not modify the given `cfg.Opts`. Such behavior is surprising and can trip up tests that reuse the same config. Also, `ResolveEncryptedEnvOptions` and `wrapFilesystemMiddleware` no longer modify the `Options` directly; and `CheckNoRegistryFile` is now a standalone function. Release note: None Epic: none 96793: kvserver: de-flake TestReplicaProbeRequest r=pavelkalinnikov a=tbg Chanced upon this failure mode in unrelated PR #96781. Epic: none Release note: None Co-authored-by: Jayant Shrivastava <[email protected]> Co-authored-by: Tobias Grieger <[email protected]> Co-authored-by: Evan Wall <[email protected]> Co-authored-by: Radu Berinde <[email protected]>

tbg force-pushed the track-raft-protos branch from d80f3b7 to 3145fc8 Compare December 16, 2022 13:35

tbg force-pushed the track-raft-protos branch from 3145fc8 to a4da07f Compare December 16, 2022 13:37

tbg marked this pull request as ready for review December 16, 2022 13:38

tbg requested a review from a team as a code owner December 16, 2022 13:38

tbg requested review from erikgrinaker and a team December 16, 2022 13:38

erikgrinaker approved these changes Dec 16, 2022

View reviewed changes

pkg/kv/kvserver/below_raft_protos_test.go Show resolved Hide resolved

pkg/kv/kvserver/below_raft_protos_test.go Show resolved Hide resolved

craig bot merged commit cc63f9f into cockroachdb:master Dec 16, 2022

tbg deleted the track-raft-protos branch December 20, 2022 10:03

andrewbaptist mentioned this pull request Apr 8, 2023

kvserver: RefreshRange performance regression #98881

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvserver: revamp raft proto test #93785

kvserver: revamp raft proto test #93785

tbg commented Dec 16, 2022

cockroach-teamcity commented Dec 16, 2022

erikgrinaker left a comment

tbg commented Dec 16, 2022

craig bot commented Dec 16, 2022

tbg commented Dec 16, 2022 •

edited

Loading

craig bot commented Dec 16, 2022

kvserver: revamp raft proto test #93785

kvserver: revamp raft proto test #93785

Conversation

tbg commented Dec 16, 2022

cockroach-teamcity commented Dec 16, 2022

erikgrinaker left a comment

Choose a reason for hiding this comment

tbg commented Dec 16, 2022

craig bot commented Dec 16, 2022

tbg commented Dec 16, 2022 • edited Loading

craig bot commented Dec 16, 2022

tbg commented Dec 16, 2022 •

edited

Loading