intentresolver: misc cleanups

[andreimatei]

See individual commits.

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Nice cleanup.

Reviewed 1 of 1 files at r1, 5 of 5 files at r2, 1 of 1 files at r3, 4 of 4 files at r4, 1 of 1 files at r5, 7 of 7 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei)

---

pkg/kv/kvserver/concurrency/lock_table_waiter.go, line 128 at r2 (raw file):

	// ResolveIntent synchronously resolves the provided intent.
	ResolveIntent(context.Context, roachpb.LockUpdate, intentresolver.ResolveOptions) *Error

👎 to this second commit. I wouldn't make it unless we need to, and the commit message is not very convincing. The entire concurrency package works on *roachpb.Error because it's awkward everywhere that we mix them (as you're noticing). I wouldn't make this change unless you're committed to switching the concurrency package and the txnwait package over to errors as well. Otherwise, we're just shifting ugly code around for no actual benefit.

---

pkg/kv/kvserver/intentresolver/intent_resolver.go, line 774 at r3 (raw file):

// ResolveOptions is used during intent resolution.
type ResolveOptions struct {
	// If set, the ranges containing the intents are to be poisoned.

Is there a good explanation of "poisoning a range" anywhere? Is so, could you reference that here? If not, is this a good place for one?

---

pkg/kv/kvserver/intentresolver/intent_resolver_test.go, line 925 at r5 (raw file):

func respForResolveIntentBatch(
	t *testing.T, ba roachpb.BatchRequest, checkTxnStatusOpt checkTxnStatusOpt,

Would it make more sense to just specify the status that we expect the intents to have? Is that possible?

---

pkg/kv/kvserver/intentresolver/intent_resolver_test.go, line 227 at r6 (raw file):

				close(done)
			}
			txn := c.txn

Do we want to clone this?

[andreimatei]

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

---

pkg/kv/kvserver/concurrency/lock_table_waiter.go, line 128 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

👎 to this second commit. I wouldn't make it unless we need to, and the commit message is not very convincing. The entire concurrency package works on *roachpb.Error because it's awkward everywhere that we mix them (as you're noticing). I wouldn't make this change unless you're committed to switching the concurrency package and the txnwait package over to errors as well. Otherwise, we're just shifting ugly code around for no actual benefit.

The reason I've done it is because I'll be adding ResolveIntent to the rangefeed.TxnPusher iface, to replace CleanupTxnIntentsAsync. CleanupTxnIntentsAsync already returns error, and I don't want to move the rangefeed package backwards.
Does that change your mind?

I wouldn't have gone ahead to do this change if it wasn't for the future plans, but generally, to me, moving from pErr->err when possible is almost always good. Even if a package deals with pErr a lot, it also probably also deals with err a lot anyway. The awkwardness to transforms err->pErr is lower than the other way, at least to me. In this particular case, the commit results in a net -2 calls to roachpb.NewError() and -1 .GoError().

[nvanbenschoten]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei)

---

pkg/kv/kvserver/concurrency/lock_table_waiter.go, line 128 at r2 (raw file):

I don't want to move the rangefeed package backwards.

Agreed, this is definitely not something we want to do.

But you're not going to be adding ResolveIntents to the interface directly, right? You're going to be calling into the IntentResolver from rangefeedTxnPusher. So you can perform the .GoError() there and not leak anything into the rangefeed package.

The way I see it, if you make this change then we now have one more package that's mixing pErr and err.

[andreimatei]

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

---

pkg/kv/kvserver/concurrency/lock_table_waiter.go, line 128 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I don't want to move the rangefeed package backwards.

Agreed, this is definitely not something we want to do.

But you're not going to be adding ResolveIntents to the interface directly, right? You're going to be calling into the IntentResolver from rangefeedTxnPusher. So you can perform the .GoError() there and not leak anything into the rangefeed package.

The way I see it, if you make this change then we now have one more package that's mixing pErr and err.

removed the commit, under protest

---

pkg/kv/kvserver/intentresolver/intent_resolver.go, line 774 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is there a good explanation of "poisoning a range" anywhere? Is so, could you reference that here? If not, is this a good place for one?

added a comment

---

pkg/kv/kvserver/intentresolver/intent_resolver_test.go, line 925 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Would it make more sense to just specify the status that we expect the intents to have? Is that possible?

done, see now

---

pkg/kv/kvserver/intentresolver/intent_resolver_test.go, line 227 at r6 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do we want to clone this?

it wasn't technically generally since we overwrite the LockSpans slice below every time, but yeah I've added a clone.

[nvanbenschoten]

Reviewed 2 of 3 files at r8, 2 of 4 files at r9, 7 of 7 files at r11.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei)

---

pkg/kv/kvserver/intentresolver/intent_resolver_test.go, line 851 at r11 (raw file):

	checkTxnAborted checkTxnStatusOpt = true

	// NOTE: There shouldbe a checkTxnCommitted option, but no test currently uses

should be

[andreimatei]

bors r+

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

---

pkg/kv/kvserver/intentresolver/intent_resolver_test.go, line 851 at r11 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

should be

done

[craig]

Build succeeded:

• GitHub CI (Cockroach)