kvserver: fully adopt useReproposalsV2#106750
Merged
craig[bot] merged 18 commits intocockroachdb:masterfrom Jul 19, 2023
Merged
kvserver: fully adopt useReproposalsV2#106750craig[bot] merged 18 commits intocockroachdb:masterfrom
craig[bot] merged 18 commits intocockroachdb:masterfrom
Conversation
So glad to see this go.
…WithNewLeaseIndexError These tests don't work with v2 and aren't maintainable or super useful anyway. We have good randomized coverage of these code paths now via kvnemesis and TestProposalsWithInjectedLeaseIndexAndReproposalError, and adding better unit test coverage is tracked in cockroachdb#106504. In addition to this, it would be desirable to extend this unit test coverage upwards through the stack via cockroachdb#105177.
Member
There are some invariants waiting to happen here.
17df203 to
20e033c
Compare
erikgrinaker
approved these changes
Jul 18, 2023
Contributor
erikgrinaker
left a comment
There was a problem hiding this comment.
LGTM, thanks for the cleanup!
The comments will be fixed up separately. This change is mechanical.
The semantics are simpler now.
useReproposalsV2 == true.
I can't think of reasons why endCmds.done (which releases latches, etc) should be called twice. Document that expectation with a TODO to verify it.
It's nice to see all of this complexity melt away.
93fc06c to
d795e6d
Compare
Member
Author
|
TFTR! Comments addressed. bors r=erikgrinaker |
Contributor
|
Build failed: |
Member
Author
|
TestRules/join_order flaked, pointed it out here bors r=erikgrinaker |
Contributor
|
Build succeeded: |
pav-kv
added a commit
to pav-kv/cockroach
that referenced
this pull request
Aug 16, 2023
Since cockroachdb#106750, ProposalData is being copied on superseding reproposals. The caller only knows about the original proposal, and only detaches its context / tracing span when abandoning the request (e.g. on timeout). By the time of abandoning the request, there might have been a few superseding reproposals, and the context / tracing span is being used by multiple ProposalData objects. In addition to rewriting the original proposal.ctx, we should do the same for all the ProposalData objects corresponding to the same request. This commit adds links between consecutive reproposals, and walks the resulting linked list of proposals when cleaning up / abandoning the request. Epic: CRDB-25287 Release note: none
pav-kv
added a commit
to pav-kv/cockroach
that referenced
this pull request
Aug 21, 2023
Since cockroachdb#106750, ProposalData is being copied on superseding reproposals. The caller only knows about the original proposal, and only detaches its context / tracing span when abandoning the request (e.g. on timeout). By the time of abandoning the request, there might have been a few superseding reproposals, and the context / tracing span is being used by multiple ProposalData objects. In addition to rewriting the original proposal.ctx, we should do the same for all the ProposalData objects corresponding to the same request. This commit unbinds the context for older proposals when reproposing them, and unbinds the latest reproposal context when cleaning up / abandoning the request. Epic: CRDB-25287 Release note: none
pav-kv
added a commit
to pav-kv/cockroach
that referenced
this pull request
Aug 21, 2023
Since cockroachdb#106750, ProposalData is being copied on superseding reproposals. The caller only knows about the original proposal, and only detaches its context / tracing span when abandoning the request (e.g. on timeout). By the time of abandoning the request, there might have been a few superseding reproposals, and the context / tracing span is being used by multiple ProposalData objects. In addition to rewriting the original proposal.ctx, we should do the same for all the ProposalData objects corresponding to the same request. This commit unbinds the context for older proposals when reproposing them, and unbinds the latest reproposal context when cleaning up / abandoning the request. Epic: CRDB-25287 Release note: none
craig bot
pushed a commit
that referenced
this pull request
Aug 21, 2023
108483: roachtest: wait smaller duration before timining out lease prefs r=erikgrinaker a=kvoli Previously, the `lease-preferences` roachtests would fail on the test tiemout, despite the test actually failing much earlier. 5 minutes stopping a node, the node is considered dead, and up-replication will begin. If the cluster doesn't satisfy lease preferences within that 5 minute window, the test will reliably fail -- making the remainder of the test timeout (30 or 60 minutes), wasted time. Fail earlier, by adhering to a post event timeout. This timeout set to 10 minutes. Informs: #108425 Epic: none Release note: None 108775: kvserver: fix double span Finish on reproposals r=erikgrinaker a=pavelkalinnikov Since #106750, `ProposalData` is being copied on superseding reproposals. The caller only knows about the original proposal, and only detaches its context / tracing span when abandoning the request (e.g. on timeout). By the time of abandoning the request, there might have been a few superseding reproposals, and the context / tracing span is being used by multiple `ProposalData` objects. In addition to rewriting the original `proposal.ctx`, we should do the same for all the `ProposalData` objects corresponding to the same request. This commit unbinds the context for older proposals when reproposing them, and unbinds the latest reproposal context when cleaning up / abandoning the request. Fixes #107521 Fixes #108534 Fixes #108241 Fixes #108663 Fixes #108696 Fixes #108837 Epic: CRDB-25287 Release note: none 108942: backupccl: deflake memory monitor restore test r=rhu713 a=rhu713 Deflake the TestRestoreMemoryMonitoring tests by only asserting the lower bound in the number of files produced in the backup to account for elastic CPU limiter. Fixes: #108239 Release note: None 109040: server: add more info to the combined statement endpoint r=maryliag a=maryliag Previously, it was hard to know which tables were used to populate the SQL Activity, making debug for it complicated. This commit adds extra information to the `combinedstmts` to help: - oldestAggregatedTsReturned - stmtsSourceTable - txnsSourceTable Returning a value for `olderAggregatedTsReturned` will also allow us to show proper warning when the older timestamp doesn't match the start date of the selected time period on the Search Criteria. Part Of #108989 Release note: None Co-authored-by: Austen McClernon <austen@cockroachlabs.com> Co-authored-by: Pavel Kalinnikov <pavel@cockroachlabs.com> Co-authored-by: Rui Hu <rui@cockroachlabs.com> Co-authored-by: maryliag <marylia@cockroachlabs.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
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.
This hard-codes useReproposalsV2 to true (prior to this PR, it's metamorphic with a default of true) and applies the resulting simplifications across the codebase.
These include simplified control flow and an overall simplified architecture.
Closes #105625.
Epic: CRDB-25287
Release note: None