kv: improve TestTxnCoordSenderRetries
#102086
Conversation
|
This change is |
nvanbenschoten
force-pushed
the
nvanbenschoten/improveTxnRestartTest
branch
from
3935598 to
76ea6e0
Compare
nvanbenschoten
force-pushed
the
nvanbenschoten/improveTxnRestartTest
branch
from
76ea6e0 to
98f97cf
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/kv/kvclient/kvcoord/dist_sender_server_test.go line 2438 at r2 (raw file):
}, { name: "write too old with get in the clear",
Unrelated to your patch, how is this case different from the priorReads=true case?
pkg/kv/kvclient/kvcoord/dist_sender_server_test.go line 2800 at r2 (raw file):
return err }, expClientRestart: true, // can't refresh
Do you think it's worth add an expectation for RefreshFail and checking it every time there is a client restart?
This commit updates TestTxnCoordSenderRetries to use a local kv.DB with a local TxnMetrics for its test transaction. This allows the test to precisely assert on the metrics without having to worry about other transactions in the system affecting them. Epic: None Release note: None
This commit adds more testing conditions to TestTxnCoordSenderRetries. Epic: None Release note: None
This was the same as the "write too old with put after prior read" case. Epic: None Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/improveTxnRestartTest
branch
from
98f97cf to
c59d66a
Compare
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/kv/kvclient/kvcoord/dist_sender_server_test.go line 2438 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Unrelated to your patch, how is this case different from the
priorReads=truecase?
Good point, it's the same test. I removed it.
pkg/kv/kvclient/kvcoord/dist_sender_server_test.go line 2800 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Do you think it's worth add an expectation for
RefreshFailand checking it every time there is a client restart?
That's a good idea. I'll do it in a separate PR though, because I have a sequence of stacked changes to this test that would be impacted by the new assertion.
|
Build succeeded: |
This commit adds explicit testing of refresh failures to `TestTxnCoordSenderRetries`, as suggested in cockroachdb#102086. Epic: None Release note: None
This commit adds explicit testing of refresh failures to `TestTxnCoordSenderRetries`, as suggested in cockroachdb#102086. Epic: None Release note: None
102670: kv: test for refresh failures in `TestTxnCoordSenderRetries` r=nvanbenschoten a=nvanbenschoten This commit adds explicit testing of refresh failures to `TestTxnCoordSenderRetries`, as suggested in #102086. Epic: None Release note: None Co-authored-by: Nathan VanBenschoten <[email protected]>
This PR improves
TestTxnCoordSenderRetriesin two ways, in preparation for its use to be expanded to demonstrate the difference in behavior between isolation levels.use local TxnMetrics in TestTxnCoordSenderRetries
The first commit updates
TestTxnCoordSenderRetriesto use a localkv.DBwith a localTxnMetricsfor its test transaction. This allows the test to precisely assert on the metrics without having to worry about other transactions in the system affecting them.add preemptive refreshes and 1PCs to TestTxnCoordSenderRetries
The second commit adds more testing conditions to TestTxnCoordSenderRetries. Each test now asserts it expectations for preemptive refreshes and for 1-phase commits.