supply local time to InternalPushTxn when initiating the request #1102
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
| expiry := args.Timestamp | ||
| // Compute heartbeat expiration (all replicas must see the same result). | ||
| expiry := args.Now // caller can set this to his wall time. | ||
| expiry.Forward(args.Timestamp) // if Now is not set, fallback |
There was a problem hiding this comment.
Is there any reason to send a push txn request without setting Now? It might be simpler to require that Now be set in all cases. (On the other hand, this does ensure that we use max(Now, Timestamp) if there is any risk that the pusher's clock could be behind)
|
LGTM. Do we also need to set Now when pushing from gc_queue.go? (which looks like the only other non-test use of InternalPushTxnRequest) |
previously args.Timestamp was used, but there are situations in which that never changes, meaning that infinite retry loops on an abandoned Txn descriptor would ensue in storage/store, for instance in cockroachdb#877 and the bank example.
|
I've made |
| {nil, 0, proto.PUSH_TIMESTAMP, false}, | ||
| {nil, 0, proto.ABORT_TXN, false}, | ||
| {nil, 0, proto.CLEANUP_TXN, false}, | ||
| {nil, 1, proto.PUSH_TIMESTAMP, false}, // using 0 is awkward |
There was a problem hiding this comment.
This comment won't make sense in the future: using 0 for what?
|
LGTM |
tbg
added a commit
that referenced
this pull request
May 26, 2015
supply local time to InternalPushTxn when initiating the request
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
Mar 1, 2019
This commit deprecates PushTxnRequest.Now and gives its responsibility to the batch header timestamp. The biggest reason to do this is so that PushTxn requests properly update their receiver's clock. This is critical because a PushTxn request can result in a timestamp cache entry to be created with a value up to this time, so for safety, we need to ensure that the leaseholder updates its clock at least to this time _before_ evaluating the request. Otherwise, a lease transfer could miss the request's effect on the timestamp cache and result in a lost push/abort. The comment on PushTxnRequest.Now mentioned that the header timestamp couldn't be used because the header's timestamp "does not necessarily advance with the node clock across retries and hence cannot detect abandoned transactions." This dates back all the way to cockroachdb#1102. I haven't been able to piece together what kind of retries this is referring to, but I'm almost positive that they don't still apply. Release note: None
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
Mar 2, 2019
This commit deprecates PushTxnRequest.Now and gives its responsibility to the batch header timestamp. The biggest reason to do this is so that PushTxn requests properly update their receiver's clock. This is critical because a PushTxn request can result in a timestamp cache entry to be created with a value up to this time, so for safety, we need to ensure that the leaseholder updates its clock at least to this time _before_ evaluating the request. Otherwise, a lease transfer could miss the request's effect on the timestamp cache and result in a lost push/abort. The comment on PushTxnRequest.Now mentioned that the header timestamp couldn't be used because the header's timestamp "does not necessarily advance with the node clock across retries and hence cannot detect abandoned transactions." This dates back all the way to cockroachdb#1102. I haven't been able to piece together what kind of retries this is referring to, but I'm almost positive that they don't still apply. Release note: None
pav-kv
pushed a commit
to pav-kv/cockroach
that referenced
this pull request
Mar 5, 2024
raft: add a one node bench
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.
previously args.Timestamp was used, but there are situations
in which that never changes, meaning that infinite retry loops
on an abandoned Txn descriptor would ensue in storage/store,
for instance in #877 and the bank example.
This needs a test. Should be fairly straightforward, so if anybody wants to help out the debugging bonanza, that would be appreciated (but probably want to wait for feedback to see if this would actually land).
After this change, the following hums along relatively nicely on a three node cluster:
/bank --num-accounts=2 --num-parallel-transfers=2 --db-name=http://root@localhost:8001Every couple of minutes one of the writers' transactions will still deadlock, but times out after
10 seconds (after which process resumes). When that happens, the busy retry loop will run against it
approximately 14000 times until the 10s are over (yeah, it's really busy - 0s backoff, I think). I've checked all the pushes in such an example, and none of them go through since the priority is pretty high:
So we only have to figure out why the occasional transaction in the bank example doesn't finish - it clearly seems to be pretty high priority wise. I've checked the logs, it doesn't seem to ever have unsuccessfully pushed someone during my run: