kv: add all lock strengths and durabilities to TestLockTableConcurrentRequests #111465
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/kv/kvserver/concurrency/lock_table_test.go line 747 at r1 (raw file):
// latch at max timestamp. See // https://github.com/cockroachdb/cockroach/issues/102264. return spanset.SpanReadOnly, hlc.MaxTimestamp
nit: Should we add a comment explaining why we don't need to duplicate the special case for replicated shared locks here?
pkg/kv/kvserver/concurrency/lock_table_test.go line 1482 at r1 (raw file):
keys = append(keys, roachpb.Key(string(rune('a'+i)))) } strs := []lock.Strength{lock.None, lock.Shared, lock.Exclusive, lock.Intent}
nit: should we pull this behind a method in locking.go, kind of like we do for isolation levels? I was thinking about it earlier today when looking at TestTxnCoordSenderRetries, but then we switched approaches.
…tRequests Informs cockroachdb#91545. Informs cockroachdb#100193. This commit expands TestLockTableConcurrentRequests to support Shared, Exclusive, and Intent locking strength, in addition to both Replicated and Unreplicated locking durabilities. This provides randomized coverage of the lock table with these combinations. The commit then temporarily disables Shared locks in the test, which occasionally fail with the panic: `tryMakeNewDistinguished called with new claimant txn`. This is related to cockroachdb#111144, so we can re-enable Shared locks when that issue is resolved. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/lockTableRandomLocks
branch
from
086d7f1 to
1448128
Compare
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/kv/kvserver/concurrency/lock_table_test.go line 747 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: Should we add a comment explaining why we don't need to duplicate the special case for replicated shared locks here?
Done.
pkg/kv/kvserver/concurrency/lock_table_test.go line 1482 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: should we pull this behind a method in
locking.go, kind of like we do for isolation levels? I was thinking about it earlier today when looking atTestTxnCoordSenderRetries, but then we switched approaches.
I considered the same thing, but it wasn't clear what we wanted to do with lock.Update. Should we just exclude it? I'll leave like this for now, but we can reconsider in a targetted PR if we think it could clean up some code.
|
Build succeeded: |
Informs #91545.
Informs #100193.
This commit expands
TestLockTableConcurrentRequeststo support Shared, Exclusive, and Intent locking strength, in addition to both Replicated and Unreplicated locking durabilities. This provides randomized coverage of the lock table with these combinations.The commit then temporarily disables Shared locks in the test, which occasionally fail with the panic:
tryMakeNewDistinguished called with new claimant txn. This is related to #111144, so we can re-enable Shared locks when that issue is resolved.Release note: None