kv: assert an absence of retries in TestTxnCoordSenderRetries #35112
Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom Feb 21, 2019
Merged
kv: assert an absence of retries in TestTxnCoordSenderRetries #35112craig[bot] merged 2 commits intocockroachdb:masterfrom
craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
The test wasn't really testing anything. We now set txn.Writing on the client, so it wasn't verifying what it thought it was. We switch it to assert that observed timestamps are propagated on error instead. Release note: None
The test was only asserting that cases that should observe retries did, not the cases that should not observe retries did not. Release note: None
Member
tbg
approved these changes
Feb 21, 2019
Member
tbg
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @andreimatei)
andreimatei
approved these changes
Feb 21, 2019
Contributor
andreimatei
left a comment
There was a problem hiding this comment.
LGTM
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @andreimatei)
Contributor
Author
|
bors r=tbg,andreimatei |
craig bot
pushed a commit
that referenced
this pull request
Feb 21, 2019
35081: sql: Fix test flake r=andy-kimball a=andy-kimball Filter out PushTxn/ResolveIntent trace messages, since they represent intent cleanup from earlier operations that interferes with subsequent tests. This was already being done in other variations, so this commit extends that to all the variations that are doing filtering of messages. Fixes #34911 Release note: None 35086: mvcc: don't write below provisional commit timestamp on WriteTooOld errors r=tbg a=nvanbenschoten Fixes #34853. This change fixes #34853, which appears to have been broken by 57d0201#diff-41e9e1b7829f8e25daeb564f42c26017L1514. That change made it so that intents are now written at a transaction's provisional commit timestamp instead of its original timestamp (which is fantastic!). In doing so, it removed special-purpose code to forward the `MVCCLogicalOpDetails` timestamp to the provisional commit timestamp of the transaction because it assumed that the new `writeTimestamp` variable that the change introduced already assumed this role. Unfortunately, the change allowed this `writeTimestamp` variable to regress below the transaction's provisional commit timestamp in subtle cases where a committed value ended up between the transaction's original timestamp and its provisional commit timestamp (a case that generates `WriteTooOldError`s). In this case, we were replacing the `writeTimestamp` with the committed value's timestamp + 1 logical tick. This regression allowed `MVCCWriteIntentOp`s to look like they were ignoring closed timestamps. The fix was to either introduce the special purpose code again or to avoid letting `writeTimestamp` regress in this `WriteTooOldError` code path. This PR chose the latter solution. In doing so, this PR also improves upon the benefit already provided by #32487 - it reduces the number of intents that need to be moved on transaction commit. This is because it ensures that if a transaction has an original (read) timestamp below a committed value and a provisional commit (write) timestamp above a committed value, an intent left by that transaction will be in a "committable" state without a need to rewrite it during intent resolution. Release note: None 35112: kv: assert an absence of retries in TestTxnCoordSenderRetries r=tbg,andreimatei a=nvanbenschoten The test was only asserting that cases that should observe retries did, not the cases that should not observe retries did not. The PR also fixes `TestPropagateTxnOnError`, which wasn't really testing anything. We now set txn.Writing on the client, so it wasn't verifying what it thought it was. We switch it to assert that observed timestamps are propagated on error instead. 35118: storage: Downgrade a spammy log message r=tbg a=bdarnell This log message only occurs on clusters with multiple stores per node, but on those clusters it fires multiple times per second. Release note: None Co-authored-by: Andrew Kimball <andyk@cockroachlabs.com> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com> Co-authored-by: Ben Darnell <ben@bendarnell.com>
Contributor
Build succeeded |
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.
The test was only asserting that cases that should observe retries did, not the cases that should not observe retries did not.
The PR also fixes
TestPropagateTxnOnError, which wasn't really testing anything. We now set txn.Writing on the client, so it wasn't verifying what it thought it was. We switch it to assert that observed timestamps are propagated on error instead.