storage: fix the setting of br.Timestamp #44411
Conversation
|
This change is |
andreimatei
force-pushed
the
txn.rationalize-br-timestamp
branch
3 times, most recently
from
d4f1a45 to
604dbd8
Compare
andreimatei
changed the title
andreimatei
force-pushed
the
txn.rationalize-br-timestamp
branch
2 times, most recently
from
720cd8d to
b566254
Compare
There was a problem hiding this comment.
now r1 and r2 are #44407
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 4 of 4 files at r2, 2 of 2 files at r3, 1 of 2 files at r4.
Reviewable status:
pkg/storage/replica_evaluate.go, line 350 at r4 (raw file):
// Update the batch response timestamp field to the timestamp at which the // batch's reads were evaluated. if !br.Timestamp.IsEmpty() {
Is this worth asserting? We created the object in this function.
pkg/storage/replica_evaluate.go, line 357 at r4 (raw file):
br.Txn = baHeader.Txn // Note that br.Txn.ReadTimestamp might be higher than baHeader.Timestamp if // we had an EndTxn that decided that it can essentially refresh to
s/essentially//
Let's just move towards calling this a "server-side refresh". Same thing in cmd_end_transaction.go.
andreimatei
force-pushed
the
txn.rationalize-br-timestamp
branch
from
b566254 to
5bb386d
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/storage/replica_evaluate.go, line 350 at r4 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Is this worth asserting? We created the object in this function.
removed
pkg/storage/replica_evaluate.go, line 357 at r4 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
s/essentially//Let's just move towards calling this a "server-side refresh". Same thing in
cmd_end_transaction.go.
done
Fix an incorrect comment on br.Timestamp that mistakenly indicated that the field is only relevant for non-transactional requests. Release note: None
Before this patch, evaluateBatch() had special code to deal with transactions committed at a different timestamp than txn.ReadTimestamp. This was useful when the EndTxn request decided that it can commit despite a bumped write timestamp. The code was broken because it was not dealing with STAGING txns. This patch simplifies this code by having EndTxn update the read timestamp. This way, evaluateBatch() no longer needs to consider the transaction's disposition. The burden is moved to EndTxn, which is already in this dirty business. The change is also in the spirit of how timestamps are supposed to work. For transactional requests, code should generally look at ba.Txn.ReadTimestamp, not at ba.Timestamp. However, evaluateBatch() was using ba.Timestamp around the affected code. Release note: None
andreimatei
force-pushed
the
txn.rationalize-br-timestamp
branch
from
5bb386d to
b04f474
Compare
There was a problem hiding this comment.
bors r+
Reviewable status:
Build failed |
|
flaked on #43949 bors r+ |
Build failed (retrying...) |
44411: storage: fix the setting of br.Timestamp r=andreimatei a=andreimatei Before this patch, evaluateBatch() had special code to deal with transactions committed at a different timestamp than txn.ReadTimestamp. This was useful when the EndTxn request decided that it can commit despite a bumped write timestamp. The code was broken because it was not dealing with STAGING txns. This patch simplifies this code by having EndTxn update the read timestamp. This way, evaluateBatch() no longer needs to consider the transaction's disposition. The burden is moved to EndTxn, which is already in this dirty business. The change is also in the spirit of how timestamps are supposed to work. For transactional requests, code should generally look at ba.Txn.ReadTimestamp, not at ba.Timestamp. However, evaluateBatch() was using ba.Timestamp around the affected code. Release note: None Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
Build succeeded |
Before this patch, evaluateBatch() had special code to deal with
transactions committed at a different timestamp than txn.ReadTimestamp.
This was useful when the EndTxn request decided that it can commit
despite a bumped write timestamp. The code was broken because it was not
dealing with STAGING txns.
This patch simplifies this code by having EndTxn update the read
timestamp. This way, evaluateBatch() no longer needs to consider the
transaction's disposition. The burden is moved to EndTxn, which is
already in this dirty business.
The change is also in the spirit of how timestamps are supposed to work.
For transactional requests, code should generally look at
ba.Txn.ReadTimestamp, not at ba.Timestamp. However, evaluateBatch() was
using ba.Timestamp around the affected code.
Release note: None