kv: stop using BeginTransaction requests #33566
Conversation
|
This change is |
nvanbenschoten
force-pushed
the
nvanbenschoten/lazyPt3
branch
5 times, most recently
from
68267f1 to
86e9a73
Compare
nvanbenschoten
force-pushed
the
nvanbenschoten/lazyPt3
branch
from
86e9a73 to
1e14ee4
Compare
nvanbenschoten
force-pushed
the
nvanbenschoten/lazyPt3
branch
from
1e14ee4 to
c7c81e5
Compare
There was a problem hiding this comment.
LGTM
✌️
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/kv/txn_interceptor_heartbeat.go, line 54 at r1 (raw file):
gatekeeper lockedSender st *cluster.Settings
nit: I think we should do something better than passing a Settings object just to figure out if (and when) some version becomes active. This pattern is unfortunately used everywhere.
Take it or leave it.
pkg/roachpb/batch.go, line 242 at r1 (raw file):
return false } nextSeq := int32(1)
mind adding a comment here saying in english that we're checking that seq numbers aren't skipped
pkg/settings/cluster/cockroach_versions.go, line 310 at r1 (raw file):
}, { // VersionLazyTxnRecord is TODO.
33566
Based on cockroachdb#33523. Closes cockroachdb#25437. This is the final part of addressing cockroachdb#32971. The change gates the use of BeginTransaction on a cluster version. When a cluster's version is sufficiently high, it will stop sending them. To make this work, a small change was needed for the detection of 1PC transactions. We now use sequence numbers to determine that a batch includes all of the writes in a transaction. This is in line with the proposal from the RFC's `1PC Transactions` section and will work correctly with parallel commits. Release note (performance improvement): Reduce the network and storage overhead of multi-Range transactions.
nvanbenschoten
force-pushed
the
nvanbenschoten/lazyPt3
branch
from
c7c81e5 to
c5516ef
Compare
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/kv/txn_interceptor_heartbeat.go, line 54 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
nit: I think we should do something better than passing a Settings object just to figure out if (and when) some version becomes active. This pattern is unfortunately used everywhere.
Take it or leave it.
What specifically don't you like about the pattern? Seems better than having a global floating around.
pkg/roachpb/batch.go, line 242 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
mind adding a comment here saying in english that we're checking that seq numbers aren't skipped
Done.
pkg/settings/cluster/cockroach_versions.go, line 310 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
33566
Done.
There was a problem hiding this comment.
Reviewable status:
pkg/kv/txn_interceptor_heartbeat.go, line 54 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
What specifically don't you like about the pattern? Seems better than having a global floating around.
I'd like something more specific to getting a version, that can easily be mocked out by tests.
Like, the reason why I've been sending the PRs around deleting migrations originated with me not wanting to pass some setting around, because those settings used to come from a Store and I was trying to divorce some code from the Store and make it more self-contained.
Anyway, not a big deal and not something that should hold up this PR in particular.
|
I verified that this passes bors r+ 🤞 |
33566: kv: stop using BeginTransaction requests r=nvanbenschoten a=nvanbenschoten Based on #33523. Closes #25437. This is the final part of addressing #32971. The change gates the use of BeginTransaction on a cluster version. When a cluster's version is sufficiently high, it will stop sending them. To make this work, a small change was needed for the detection of 1PC transactions. We now use sequence numbers to determine that a batch includes all of the writes in a transaction. This is in line with the proposal from the RFC's `1PC Transactions` section and will work correctly with parallel commits. Release note (performance improvement): Reduce the network and storage overhead of multi-Range transactions. Co-authored-by: Nathan VanBenschoten <[email protected]>
Build succeeded |
Based on #33523.
Closes #25437.
This is the final part of addressing #32971.
The change gates the use of BeginTransaction on a cluster version. When a cluster's version is sufficiently high, it will stop sending them.
To make this work, a small change was needed for the detection of 1PC transactions. We now use sequence numbers to determine that a batch includes all of the writes in a transaction. This is in line with the proposal from the RFC's
1PC Transactionssection and will work correctly with parallel commits.Release note (performance improvement): Reduce the network and storage
overhead of multi-Range transactions.