storage: return error from split/merge lock acquisition

[tbg]

See #19172.

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

pkg/storage/store.go, line 3878 at r1 (raw file):

			//
			// Inspired by https://github.com/cockroachdb/cockroach/pull/19404.
			runtime.Gosched()

Gosched can hurt performance, especially in the tail latencies, so I wouldn't say there's no harm. That said, this shouldn't be very common so I doubt it will have an impact. Still, I'm not sure it's worth the risk if there's no reason to add this.

---

Comments from Reviewable

[tbg]

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

pkg/storage/store.go, line 3878 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Gosched can hurt performance, especially in the tail latencies, so I wouldn't say there's no harm. That said, this shouldn't be very common so I doubt it will have an impact. Still, I'm not sure it's worth the risk if there's no reason to add this.

Note that the Gosched is on the err = errRetry path, so I doubt it matters for performance. And, with a loop like this, I can't prove that there's no reason to add this, and unless we can't, I think we should generally add it to avoid this kind of freak deadlock.

---

Comments from Reviewable

[petermattis]

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

---

pkg/storage/replica.go, line 4536 at r2 (raw file):

	rightRng, created, err := r.store.getOrCreateReplica(ctx, split.RightDesc.RangeID, 0, nil)
	if err != nil {
		log.Fatalf(ctx, "while acquiring split lock: %s", err)

Shouldn't you return the error to the caller in order to force error the Raft command? The code in Store.tryGetOrCreateReplica has enough error paths that I can't convince myself none of them will occur.

---

pkg/storage/store.go, line 3878 at r1 (raw file):

			//
			// Inspired by https://github.com/cockroachdb/cockroach/pull/19404.
			runtime.Gosched()

Was this addition motivated by a real concern. Store.tryGetOrCreateReplica performs multiple lock operations which will definitely provide stop-the-world safe points for the GC.

---

Comments from Reviewable

[tbg]

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.

---

pkg/storage/replica.go, line 4536 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Shouldn't you return the error to the caller in order to force error the Raft command? The code in Store.tryGetOrCreateReplica has enough error paths that I can't convince myself none of them will occur.

Good point. I definitely want to learn about the errors out there in the wild though, so I added reporting to sentry. PTAL!

---

pkg/storage/store.go, line 3878 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Was this addition motivated by a real concern. Store.tryGetOrCreateReplica performs multiple lock operations which will definitely provide stop-the-world safe points for the GC.

No, just paranoia given the recent deadlock in TestRefreshPendingCmds. You're right that the abundance of lock acquisition makes this too paranoid. Removed the commit.

---

Comments from Reviewable

[bdarnell]

---

Reviewed 2 of 4 files at r3.
Review status: 2 of 3 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[nvanbenschoten]

LGTM, but the PR title is incorrect now.

[petermattis]

---

Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

Comments from Reviewable

[tbg]

TFTRs, everyone! @nvanbenschoten changed the title.

[nvanbenschoten]

I stumbled upon this PR while cleaning up #38954 and am now confused about why setting forcedErr to the result of maybeAcquireSplitMergeLock is correct. We don't expect to ever see an error here, but if we do, I don't think we can silently let it prevent one of the replicas from applying the split/merge. That's the kind of thing that causes replica inconsistencies. I think the only option we have is to fatal.

[tbg]

Not sure what we were thinking there, Fatal looks like the only option.