storage: Add comments about leases and liveness

[bdarnell]

I had to study this code for #35986, so I took the opportunity to
update the documentation while it's fresh in my mind.

Release note: None

[cockroach-teamcity]

This change is 

[tbg]

Reviewed 4 of 4 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @bdarnell)

---

pkg/storage/node_liveness.go, line 132 at r1 (raw file):

it is

---

pkg/storage/node_liveness.go, line 145 at r1 (raw file):

// not working. Transactions were used only to piggyback on the
// transaction commit trigger). The leaseholder of the liveness range
// gossips its contents whenever they change; no other node reads from

You could mention that we don't blindly gossip everything but attempt to gossip only that which has changed.

---

pkg/storage/node_liveness.go, line 146 at r1 (raw file):

// transaction commit trigger). The leaseholder of the liveness range
// gossips its contents whenever they change; no other node reads from
// this range directly.

cockroach/pkg/storage/node_liveness.go

Lines 266 to 269 in a134caa

	var oldLiveness storagepb.Liveness
	if err := nl.db.GetProto(ctx, keys.NodeLivenessKey(nodeID), &oldLiveness); err != nil {
	return false, errors.Wrap(err, "unable to get liveness")
	}

---

pkg/storage/node_liveness.go, line 544 at r1 (raw file):

// This method is rarely called directly; heartbeats are normally sent
// by the StartHeartbeat loop.
// TODO(bdarnell): Should we just remove this synchronous heartbeat completely?

Sounds like it, though you'll probably run into trouble actually doing so.

---

pkg/storage/node_liveness.go, line 620 at r1 (raw file):

		// expired while in flight, so maybe we don't have to care about
		// that and only need to distinguish between same and different
		// epochs in our return value.

Anything in git blame for this check?

---

pkg/storage/replica_range_lease.go, line 25 at r1 (raw file):

, and any

[bdarnell]

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

---

pkg/storage/node_liveness.go, line 132 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

it is

Done.

---

pkg/storage/node_liveness.go, line 145 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

You could mention that we don't blindly gossip everything but attempt to gossip only that which has changed.

Done.

---

pkg/storage/node_liveness.go, line 146 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

cockroach/pkg/storage/node_liveness.go

Lines 266 to 269 in a134caa

	var oldLiveness storagepb.Liveness
	if err := nl.db.GetProto(ctx, keys.NodeLivenessKey(nodeID), &oldLiveness); err != nil {
	return false, errors.Wrap(err, "unable to get liveness")
	}

Oops. Changed to "other nodes rarely read from this range directly", and added a todo to consider making the stronger form true.

---

pkg/storage/node_liveness.go, line 620 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Anything in git blame for this check?

It's been there since the beginning (#10305) without much discussion.

---

pkg/storage/replica_range_lease.go, line 25 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

, and any

I mean "but any". Reworded to "The current arrangement is not perfect, and some opportunities for improvement appear, but any changes must be made very carefully".

[bdarnell]

bors r=tbg

[craig]

Timed out

[bdarnell]

bors r=tbg

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)