roachpb/storage: complete removal of BeginTransactionRequest#42933
Merged
craig[bot] merged 3 commits intocockroachdb:masterfrom Dec 4, 2019
Merged
roachpb/storage: complete removal of BeginTransactionRequest#42933craig[bot] merged 3 commits intocockroachdb:masterfrom
craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
Member
|
This change is |
97e0941 to
6a1bc91
Compare
tbg
approved these changes
Dec 4, 2019
Member
tbg
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 28 of 28 files at r3.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/storage/client_merge_test.go, line 1227 at r2 (raw file):
desctiptor
This was no longer true, as of cockroachdb#41104. Release note: None
TestStoreRangeMergeSplitRace_MergeWins was just missing a bit of cleanup that was made possible by 519bb56. TestStoreRangeMergeSplitRace_SplitWins was more badly broken. It hasn't been working for over a year and because it was broken by both 352bdf4 and 68267f1. The first commit broke the range descriptor key it expected the BeginTxn req to have and the second commit stopped sending BeginTxns entirely. However, I'm not sure how it ever worked. The test blocked intent resolution until it finished, but intent resolution on the split's meta2 intent was needed to allow the merge to proceed. My only thought is that the merge was never succeeding and was always hitting the "range changed during merge" error, so the test worked fine. Regardless, this commit fixes and cleans up both tests. I verified that I could reproduce the race conditions described by each of these tests after the fixup/cleanup. Release note: None
This commit completes the migration started in cockroachdb#32971 (a year ago to the week). It removes all references to `BeginTransactionRequest` and deletes the request type entirely. This reduces the number of request types that can create transaction records down to just two: `EndTransactionRequest` and `HeartbeatTxnRequest` (see the state transition diagram in `replica_tscache.go`). It is safe to remove server-side handling of `BeginTransactionRequest` because we never use it in v19.2 binaries, so v20.1 binaries will never run in a cluster that is using the request type. Once this commit is merged, I'm going to `s/EndTransaction/EndTxn/g` to bring that request name in line with all other transaction metadata requests (e.g. `HeartbeatTxnRequest`, `PushTxnRequest`, `QueryTxnRequest`, and `RecoverTxnRequest`). Release note: None
6a1bc91 to
128b076
Compare
nvb
commented
Dec 4, 2019
Contributor
Author
nvb
left a comment
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/storage/client_merge_test.go, line 1227 at r2 (raw file):
Previously, tbg (Tobias Grieger) wrote…
desctiptor
Done.
Contributor
Build failed |
Contributor
Author
|
Flaked on #42098. bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Dec 4, 2019
42933: roachpb/storage: complete removal of BeginTransactionRequest r=nvanbenschoten a=nvanbenschoten This commit completes the migration started in #32971 (a year ago to the week). It removes all references to `BeginTransactionRequest` and deletes the request type entirely. This reduces the number of request types that can create transaction records down to just two: `EndTransactionRequest` and `HeartbeatTxnRequest` (see the state transition diagram in `replica_tscache.go`). It is safe to remove server-side handling of `BeginTransactionRequest` because we never use it in v19.2 binaries, so v20.1 binaries will never run in a cluster that is using the request type. Once this commit is merged, I'm going to `s/EndTransaction/EndTxn/g` to bring that request name in line with all other transaction metadata requests (e.g. `HeartbeatTxnRequest`, `PushTxnRequest`, `QueryTxnRequest`, and `RecoverTxnRequest`). Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Contributor
Build succeeded |
nvb
added a commit
to nvb/cockroach
that referenced
this pull request
Dec 27, 2019
This commit renames all references to EndTransaction to EndTxn. This brings the name in line with all other transaction metadata requests (e.g. HeartbeatTxnRequest, PushTxnRequest, QueryTxnRequest, and RecoverTxnRequest), as was discussed in cockroachdb#42933. It also removes a lot of visual noise wherever the request type is used. All comments modified were re-wrapped to 80 characters. Release note: None
craig bot
pushed a commit
that referenced
this pull request
Dec 27, 2019
43387: *: rename roachpb.EndTransactionRequest to EndTxnRequest r=nvanbenschoten a=nvanbenschoten This commit renames all references to `EndTransaction` to `EndTxn`. This brings the name in line with all other transaction metadata requests (e.g. `HeartbeatTxnRequest`, `PushTxnRequest`, `QueryTxnRequest`, and `RecoverTxnRequest`), as was discussed in #42933. It also removes a lot of visual noise wherever the request type is used. All comments modified were re-wrapped to 80 characters. Co-authored-by: Nathan VanBenschoten <nvanbenschoten@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.
This commit completes the migration started in #32971 (a year ago to the week). It removes all references to
BeginTransactionRequestand deletes the request type entirely. This reduces the number of request types that can create transaction records down to just two:EndTransactionRequestandHeartbeatTxnRequest(see the state transition diagram inreplica_tscache.go).It is safe to remove server-side handling of
BeginTransactionRequestbecause we never use it in v19.2 binaries, so v20.1 binaries will never run in a cluster that is using the request type.Once this commit is merged, I'm going to
s/EndTransaction/EndTxn/gto bring that request name in line with all other transaction metadata requests (e.g.HeartbeatTxnRequest,PushTxnRequest,QueryTxnRequest, andRecoverTxnRequest).