kvclient/kvcoord: remove stale TODO in Step #156953
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
stevendanna
left a comment
stevendanna
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained
-- commits line 5 at r1:
Small typo:
Suggestion:
a now-closed issue.
pkg/kv/kvclient/kvcoord/txn_coord_sender.go line 1532 at r1 (raw file):
// if tc.typ != kv.RootTxn { // return errors.AssertionFailedf("cannot step in non-root txn") // }
Q: I mildly wonder whether we could assert that no one steps the read timestamp on a leaf transaction.
We currently have a TODO in Step to add an assertion that stepping is only done on the RootTxn which references a now-closed issue. I did a little bit archeology, and in bd255e5 we consciously removed the assertion like this since stepping on a LeafTxn should be no-op, should just work, and is actually needed behavior for internal executors when they run on remote nodes as part of DistSQL plan. (In connExecutor we currently unconditionally Step the txn on the main query path.) Thus I don't think we want to implement the TODO and should just remove it. Release note: None
yuzefovich
force-pushed
the
kv-step-todo
branch
from
40f2bde to
de735d7
Compare
yuzefovich
left a comment
yuzefovich
left a comment
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
Previously, stevendanna (Steven Danna) wrote…
Small typo:
Done.
pkg/kv/kvclient/kvcoord/txn_coord_sender.go line 1532 at r1 (raw file):
Previously, stevendanna (Steven Danna) wrote…
Q: I mildly wonder whether we could assert that no one steps the read timestamp on a leaf transaction.
On the CI only TestInternalExecutorInLeafTxnDoesNotPanic failed with that assertion. A particular reproduction from #45924 which prompted removal of the assertion a while back probably wouldn't hit the assertion anymore (there we used the internal executor to perform a cast to Oid type on a remote node, but we disallow usage of DistSQL in such case nowadays), but we now have other reasons for using LeafTxns (streamer, "parallel scans" via table readers on the same node) and I don't think we have any guarantee that we won't use the internal executor in all scenarios.
|
Build succeeded: |
3 participants
We currently have a TODO in Step to add an assertion that stepping is only done on the RootTxn which references a now-closed issue. I did a little bit archeology, and in bd255e5 we consciously removed the assertion like this since stepping on a LeafTxn should be no-op, should just work, and is actually needed behavior for internal executors when they run on remote nodes as part of DistSQL plan. (In connExecutor we currently unconditionally Step the txn on the main query path.) Thus I don't think we want to implement the TODO and should just remove it.
Epic: None
Release note: None