storage: bump LastHeartbeat timestamp when writing txn record #25034
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
Fixes cockroachdb#23945. See cockroachdb#20448. This change addresses a case where delayed BeginTxn requests can result in txn records looking inactive immediately upon being written. We now bump the txn record's LastHeartbeat timestamp when writing the record. Release note: None
|
This change is |
|
Review status: 0 of 1 files reviewed at latest revision, all discussions resolved. Comments from Reviewable |
spencerkimball
approved these changes
Apr 24, 2018
|
TFTR @bdarnell. |
|
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Apr 24, 2018
24969: opt: Hoist Exists operator and try to decorrelate r=andy-kimball a=andy-kimball This PR has two commits for easier review. The first adds support for OuterCols as a relational property. The second uses that to implement transforming the Exists operator into a SemiJoinApply operator, and then attempting to decorrelate that. 25034: storage: bump LastHeartbeat timestamp when writing txn record r=nvanbenschoten a=nvanbenschoten Fixes #23945. See #20448. This change addresses a case where delayed BeginTxn requests can result in txn records looking inactive immediately upon being written. We now bump the txn record's LastHeartbeat timestamp when writing the record. Release note: None Co-authored-by: Andrew Kimball <andyk@cockroachlabs.com> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Build succeeded |
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
Apr 15, 2019
This change increases the duration between transaction heartbeats before a transaction is considered expired from 2 seconds to 5 seconds. This has been found to dramatically reduce the frequency of transaction aborts when a cluster is under significant load. This is not expected to noticeably hurt cluster availability in the presence of dead nodes because we already have availability loss on the order of 9 seconds due to the epoch-based lease duration. This is especially important now that the hack in cockroachdb#25034 is gone. That hack was hiding some of this badness and giving transactions a bit more room to avoid being aborted. Release note: None
craig bot
pushed a commit
that referenced
this pull request
Apr 16, 2019
36748: txnwait: increase TxnLivenessThreshold significantly, reduce txn aborts under load r=nvanbenschoten a=nvanbenschoten This change increases the duration between transaction heartbeats before a transaction is considered expired from 2 seconds to 5 seconds. This has been found to dramatically reduce the frequency of transaction aborts when a cluster is under significant load. This is not expected to noticeably hurt cluster availability in the presence of dead nodes because we already have availability loss on the order of 9 seconds due to the epoch-based lease duration. This is especially important now that the hack in #25034 is gone. That hack was hiding some of this badness and giving transactions a bit more room to avoid being aborted. Here's some testing with TPC-C 2300 on AWS with 3 `c5d.4xlarge` machines. I first ran with the the 2 second TxnLivenessThreshold and then with the 5 second: <img width="872" alt="Screen Shot 2019-04-10 at 4 51 28 PM" src="https://user-images.githubusercontent.com/5438456/55926033-ae332c00-5bdd-11e9-8d6d-184ba1d5ffcd.png"> The top graph is transaction aborts and the bottom graph is p99 service latency. During my tests I actually saw an even bigger difference most of the time. In this trial, leases were still moving around because I didn't use a ramp period (which skews the txn aborts when it stops). I ran another 15 minute run again half an hour later once the leases had balanced (again with a 5 second TxnLivenessThreshold) and saw the latencies I was more accustomed to seeing with the change: <img width="869" alt="Screen Shot 2019-04-10 at 5 59 19 PM" src="https://user-images.githubusercontent.com/5438456/55926104-09651e80-5bde-11e9-9540-9d7c697a9181.png"> cc. @danhhz, who picked this up back in January when monitoring the performance of an alpha release. I dug into the changes made in 8143b45 that seemed to be causing issues with transaction aborts but never developed a good theory for what could be causing them and had trouble reliably reproducing them. Little did I know that it wasn't what was added in 8143b45 that made the difference, it was what [was removed](8143b45#diff-3c31a6497771c33db423a4cead092979L130). The first commit is from #36729 and seems like a generally good change to make. There's no reason we should be sending async aborts if we already know that the transaction was finalized. This was probably also made a little worse by 8143b45 because we now check to see whether a transaction is commitable when its record is missing when evaluating a `HeartbeatTxn` requests. Before we would simply return a `TransactionNotFoundStatusError`, which was ignored. Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
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.
Fixes #23945.
See #20448.
This change addresses a case where delayed BeginTxn requests can result
in txn records looking inactive immediately upon being written. We now
bump the txn record's LastHeartbeat timestamp when writing the record.
Release note: None