storage/report: don't deserialize zone configs over and over

[andreimatei]

This patch is expected to speedup reports generation considerably by not
unmarshalling zone config protos for every range. Instead, the
report-geneating visitors now keep state around the zone that a visited
range is in and reuse that state if they're told that the following
range is in the same zone. The range iteration infrastructure was
enhanced to figure out when zones change from range to range and tell
that to the visitors.

Without this patch, generating the reports was pinning a core for
minutes for a cluster with partitioning and 200k ranges. The profiles
showed that it's all zone config unmarshalling.

Fixes #41609

Release note (performance improvement): The performance of generating
the system.replication_* reports was greatly improved for large
clusters.

[cockroach-teamcity]

This change is 

[andreimatei]

I still need to write some new tests, but PTAL

[darinpp]

Looks good.

[ajwerner]

Seems fine to me. Mostly just nits.

Reviewed 2 of 5 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @andreimatei)

---

pkg/storage/reports/constraint_stats_report.go, line 445 at r3 (raw file):

	defer func() {
		if retErr != nil {

super nit: v.visitErr = retErr != nil?

---

pkg/storage/reports/critical_localities_report.go, line 318 at r3 (raw file):

	defer func() {
		if retErr != nil {

super nit: v.visitErr = retErr != nil

---

pkg/storage/reports/critical_localities_report.go, line 349 at r3 (raw file):

) (retErr error) {
	defer func() {
		if retErr != nil {

...

---

pkg/storage/reports/reporter.go, line 345 at r3 (raw file):

	}
	// Check that the subzone, if any, is the same.
	if r.lastZone == nil {

I don't really get this case. Are we saying that sometimes we call setZone with nil?

---

pkg/storage/reports/reporter.go, line 555 at r3 (raw file):

// visitorError is returned by visitRanges when one or more visitors failed.
type visitorError struct {

style question which I could go either way on: how about type visitorError []error

---

pkg/storage/reports/reporter.go, line 635 at r3 (raw file):

				// Sanity check - v.failed() should return an error now (the same as err above).
				if !v.failed() {
					return errors.Errorf("expected visitor %T to have failed() after error: %s", v, err)

Would this be a reasonable use case for errors.NewAssertionErrorWithWrappedErrf?

[andreimatei]

I've got a better structure now and added a bunch of tests. PTAL.

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

---

pkg/storage/reports/constraint_stats_report.go, line 445 at r3 (raw file):

Previously, ajwerner wrote…

super nit: v.visitErr = retErr != nil?

done

---

pkg/storage/reports/critical_localities_report.go, line 318 at r3 (raw file):

Previously, ajwerner wrote…

super nit: v.visitErr = retErr != nil

done

---

pkg/storage/reports/critical_localities_report.go, line 349 at r3 (raw file):

Previously, ajwerner wrote…

...

done

---

pkg/storage/reports/reporter.go, line 345 at r3 (raw file):

Previously, ajwerner wrote…

I don't really get this case. Are we saying that sometimes we call setZone with nil?

this was some nonsense

---

pkg/storage/reports/reporter.go, line 555 at r3 (raw file):

Previously, ajwerner wrote…

style question which I could go either way on: how about type visitorError []error

meh

---

pkg/storage/reports/reporter.go, line 635 at r3 (raw file):

Previously, ajwerner wrote…

Would this be a reasonable use case for errors.NewAssertionErrorWithWrappedErrf?

I don't think so. Here the original err is not a "cause" of the assertion failure; this is not a usual wrapping situation

[ajwerner]

There's something unsatisfying about the copy-pasta on each of the visitor implementations but I can live with it.

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

---

pkg/storage/reports/reporter.go, line 354 at r4 (raw file):

) (ZoneKey, error) {
	objectID, _ := config.DecodeKeyIntoZoneIDAndSuffix(rd.StartKey)
	first := true

super nit: add commentary to make it clearer to the reader that visitZones walks from the bottom up. The logic is clear once you know that fact.

---

pkg/storage/reports/reporter.go, line 389 at r4 (raw file):

// example, if the zoneChecker was previously configured for a range starting at
// /Table/51 and is now queried for /Table/52, it will say that the zones don't
// match even if in fact they do ( because neither table defines its own zone

nit: extra space.

[andreimatei]

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

---

pkg/storage/reports/reporter.go, line 354 at r4 (raw file):

Previously, ajwerner wrote…

super nit: add commentary to make it clearer to the reader that visitZones walks from the bottom up. The logic is clear once you know that fact.

done

---

pkg/storage/reports/reporter.go, line 389 at r4 (raw file):

Previously, ajwerner wrote…

nit: extra space.

done

[andreimatei]

bors r+

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

[craig]

Build succeeded

• GitHub CI (Cockroach)