spanconfig/sqlwatcher: hide factory pattern, speed up tests, surface no-op checkpoints periodically

[irfansharif]

See individual commits.

---

spanconfig/sqlwatcher: speed up tests

Using a more aggressive closed timestamp target duration brings the runtime for
TestSQLWatcherReactsToUpdate down from 45s+ to single digits. To speed it up,
we also just re-use the same SQLWatcher instead of creating a new one for every
test case.

Other tests in the package benefit from a similar treatment. Using the default
target duration in fact masked a buggy test; in a future commit we end up
rewrite that test so it's skipped for now.

spanconfig/sqlwatcher: hide the factory pattern

The only mutable state across concurrent WatchForSQLUpdate calls was the
internal buffer, which does not need to hang off the surrounding struct. This
obviates the need for the factory pattern we were using -- callers can set up
multiple SQLWatchers concurrently as is (see TestSQLWatcherMultiple).

This PR first hides the factory under the package boundary; in a later commit
it shed it altogether. This has the benefit of reducing the number of symbols
in pkg/spanconfig and making it symmetric with the other spanconfig
dependencies typically found together (KVAccessor, SQLTranslator). It's also
every-so-slightly less verbose to use in the upcoming spanconfig.Reconciler
(#71994).

spanconfig/sqlwatcher: checkpoint, even without updates

If system.{descriptor,zones} is static, we would previously discard
all checkpoint events. This is not what we want -- we'd still like to
know how caught up we are, with ongoing updates or otherwise.

MVCC GC, after all, can still happen; if our last checkpoint was when
the last update occurred, we may end up doing a lot of unnecessary work
when finding out our last checkpoint was GC-ed from underneath us. Lack
of periodic checkpoints also makes tests difficult to write -- you'd
have to induce a benign update to flush out all earlier ones. This
latent flakiness was uncovered after speeding up some existing tests in
an earlier commit.

NB: For incremental (possibly noop) checkpoints, we need to ensure that
the sqlwatcher's buffer is initialized with a low watermark. When
flushing events, we take the most conservative timestamp. If not
initialized to a high water, this might be 0 -- violating the
sqlwatcher's monotonically increasing checkpoint ts invariant.

Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Reviewed 2 of 2 files at r1, 10 of 10 files at r2, 1 of 1 files at r3, 6 of 6 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, and @irfansharif)

---

pkg/spanconfig/spanconfigsqlwatcher/sqlwatcher.go, line 165 at r4 (raw file):

				return err
			}
			if len(events) == 0 && !checkpointNoops.ShouldProcess(timeutil.Now()) {

Would it be straightforward to write a test for this?

---

pkg/spanconfig/spanconfigsqlwatcher/sqlwatcher_test.go, line 371 at r1 (raw file):

				return
			default:
				time.Sleep(100 * time.Millisecond)

Why was this needed?

---

pkg/spanconfig/spanconfigsqlwatcher/sqlwatcher_test.go, line 320 at r2 (raw file):

	}

	wg.Wait()

nit: consider pulling this call into the branch that includes the goroutines it's waiting on.

[irfansharif]

Thanks Nathan! Merging on green.

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

---

pkg/spanconfig/spanconfigsqlwatcher/sqlwatcher.go, line 165 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Would it be straightforward to write a test for this?

It would be, done.

---

pkg/spanconfig/spanconfigsqlwatcher/sqlwatcher_test.go, line 371 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Why was this needed?

We were busy looping before. The wait isn't "needed" for anything, but I don't think busy looping is what we wanted here either.

[irfansharif]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)