lockspanset: introduce LockSpanSets

[arulajmani]

Today we use spanset.SpanSet to declare both lock and latch spans. However, SpanSets do not generalize to more lock strengths. This patch introduces LockSpanSets in preperation for shared locks. We don't make use of them in the concurrency package yet; that will happen in an upcoming commit.

The main motivations for the new LockSpanSets structure are:

• A desire to store/declare lock spans using lock strengths, not span access.
• There isn't a need to store MVCC spans -- lock strengths do not have associated timestamps.
• There is no need to store global/local lock keys separately, like we do for latches.

Furthermore, we only port over methods on SpanSet that lock spans made use of in this patch.

Informs: #102008

Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Just nits. besides them.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani)

---

pkg/kv/kvserver/concurrency/lock/locking.go line 56 at r1 (raw file):

const MaxDurability = Unreplicated

// MaxStrength is the maximum value of the Strength enum.

two tiny nits:

• the Strength type is declared above the Durability type, so let's do the same for this const and its validation in the init function
• s/of/in/ to exactly mirror the comment on MaxDurability

---

pkg/kv/kvserver/lockspanset/lockspanset.go line 38 at r1 (raw file):

// GetSpans returns a slice of spans with the given access.
func (l *LockSpanSet) GetSpans(access lock.Strength) []roachpb.Span {

Is access the right name for these params? How about s or str?

---

pkg/kv/kvserver/lockspanset/lockspanset.go line 48 at r1 (raw file):

}

// SortAndDeDup sorts the spans in the SpanSet and removes any duplicates.

s/SpanSet/LockSpanSet/

Also, we're using "lock span set" in the comment above. Let's switch it over for uniformity.

---

pkg/kv/kvserver/lockspanset/lockspanset.go line 50 at r1 (raw file):

// SortAndDeDup sorts the spans in the SpanSet and removes any duplicates.
func (l *LockSpanSet) SortAndDeDup() {
	for st := lock.Strength(0); st < NumLockStrength; st++ {

What do you think of looping between 0 (inclusive) and NumLockStrength (exclusive), as opposed to a more traditional for st := range l.spans? To me, it looks more error-prone because of the assumption that len(l.spans) == NumLockStrength, but I suspect that we do it (here and in spanset) to get strong typing for the st variable. I'm not sure that's needed for a direct array access (e.g. l.spans[st]), but it is to call GetSpans. Feel free to leave this as you have it, but also feel free to switch if it's troubling you as well.

---

pkg/kv/kvserver/lockspanset/lockspanset_test.go line 48 at r1 (raw file):

	spans := lss.GetSpans(lock.None)
	require.True(t, reflect.DeepEqual(spans, []roachpb.Span{spA}))

Is require.True(t, reflect.DeepEqual(...)) different from require.Equal(t, ...)?

[arulajmani]

I realized that we needed a Copy method on LockSpanSets, so I added that as part of this patch + added a test for it. TFTR!

bors r=nvanbenschoten

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten)

---

pkg/kv/kvserver/lockspanset/lockspanset.go line 38 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is access the right name for these params? How about s or str?

Changed, here and below.

---

pkg/kv/kvserver/lockspanset/lockspanset.go line 50 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

What do you think of looping between 0 (inclusive) and NumLockStrength (exclusive), as opposed to a more traditional for st := range l.spans? To me, it looks more error-prone because of the assumption that len(l.spans) == NumLockStrength, but I suspect that we do it (here and in spanset) to get strong typing for the st variable. I'm not sure that's needed for a direct array access (e.g. l.spans[st]), but it is to call GetSpans. Feel free to leave this as you have it, but also feel free to switch if it's troubling you as well.

Ended up switching it over.

To be fair, I don't have strong feelings either way -- on one hand, the st := range l.spans is more idiomatic, but given we need the loop using (inclusive) to NumLockStrength in a few of these functions, having two different structures is sacrifices uniformity.

---

pkg/kv/kvserver/lockspanset/lockspanset_test.go line 48 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is require.True(t, reflect.DeepEqual(...)) different from require.Equal(t, ...)?

It isn't, switched it to require.Equal. I modified an existing test we have for SpanSets here which used reflect.DeepEqual and I'm guessing we didn't use require in 2017.

[nvanbenschoten]

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @arulajmani)

---

pkg/kv/kvserver/lockspanset/lockspanset.go line 50 at r1 (raw file):

given we need the loop using (inclusive) to NumLockStrength in a few of these functions

Which functions are those? I agree about uniformity being more important than the specific iteration approach in these cases, but don't see the cases can't be made uniform.

If we're switching to for st := range l.spans { in some cases, we might as well switch to for st, spans := range l.spans { in place of the remaining calls to GetSpans in a loop.

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[arulajmani]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/kv/kvserver/lockspanset/lockspanset.go line 50 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

given we need the loop using (inclusive) to NumLockStrength in a few of these functions

Which functions are those? I agree about uniformity being more important than the specific iteration approach in these cases, but don't see the cases can't be made uniform.

If we're switching to for st := range l.spans { in some cases, we might as well switch to for st, spans := range l.spans { in place of the remaining calls to GetSpans in a loop.

I thin I misunderstood your suggestion then -- I didn't realize you were suggesting we remove the GetSpans calls and replace them with l.spans. I'll add a commit to clean this up in the next PR that integrates LockSpanSets.