spanconfigsql{watcher,reconciler}: setup SQLWatcher to watch for pts updates by adityamaru · Pull Request #75122 · cockroachdb/cockroach

adityamaru · 2022-01-19T03:21:58Z

This change sets up a rangefeed on the system.pts_records table,
and adds this to the set of rangefeeds managed by the SQLWatcher. We
switch the unit emitted by the SQLWatcher to a union type called
SQLUpdate which captures either an update to a descriptor or to a
protected timestamp target. The zones and descriptor table rangefeeds
will continue to emit descriptor updates. The pts rangefeed will emit
descriptor updates for records that target schema objects, and pts target
updates for records that target cluster or tenants.

The SQLWatcher continues to dedup the SQLUpdates that it emits, so as to
ensure there is only one SQLUpdate per descriptor ID, and one SQLUpdate
per cluster or tenant target. The reconciler registers a handler that
is invoked everytime a batch of SQLUpdates are emitted by the SQLWatcher.
There is change in semantics in this part of the code, a future PR will teach
the reconciler to parse the pts target SQLUpdates, and in turn instruct the
SQLTranslator to generate the appropriate SystemSpanConfigs.

Note, this file moves the sqlwatcher tests into a ccl package so as to be
able to run backup statements to test the rangefeed on the pts table.

Informs: #73727

Release note: None

cockroach-teamcity · 2022-01-19T03:22:06Z

This change is

adityamaru · 2022-01-19T03:23:57Z

@irfansharif @arulajmani this is an early draft of what I had in mind for how the SQLWatcher would watch for protected timestamp updates. Before I clean up the tests and tidy up the PR I wanted inputs on some of the changes to the SQLWatcher interface.

adityamaru · 2022-01-27T03:01:21Z

This is RFAL now!

adityamaru · 2022-01-28T16:19:32Z

@irfansharif wrapped the target in a ProtectedTimestampUpdate - https://github.com/cockroachdb/cockroach/pull/75122/files#diff-879b43c0345e0967f0b1ee30422ea1993314a45a3195cb2cff030a62791ff434R309

Instead of storing it as separate Cluster and Tenant fields, I went with an opaque ptpb.Target field that we sanity check in the constructor. The reason I went with this is it becomes cleaner to dedup these entries in the watcher, based on the ptpb.Target stringer. Let me know what you think.

pkg/ccl/spanconfigccl/spanconfigsqlwatcherccl/sqlwatcher_test.go

pkg/spanconfig/spanconfig.go

pkg/spanconfig/spanconfigreconciler/reconciler.go

pkg/spanconfig/spanconfigsqlwatcher/buffer.go

pkg/spanconfig/spanconfigsqlwatcher/buffer_test.go

adityamaru · 2022-02-01T23:46:03Z

@irfansharif changed to unwrap the Tenants_Target into individual SQLUpdates in the SQLWatcher. Also made the sort stable, since each SQLUpdate only has one tenant ID in it now.

irfansharif

LGTM, nice work! Left only nits.

pkg/spanconfig/spanconfigsqlwatcher/buffer.go

pkg/spanconfig/spanconfigsqlwatcher/sqlwatcher.go

pkg/spanconfig/spanconfigsqlwatcher/buffer_test.go

arulajmani

I only have a few nits! Good stuff

Reviewed 1 of 13 files at r1, 10 of 12 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, and @irfansharif)

pkg/spanconfig/spanconfigsqlwatcher/buffer.go, line 109 at r2 (raw file):

// Less implements the sort.Interface methods.
func (s bufferedEvents) Less(i, j int) bool {

nit: do you think it's worth capturing the sort order here? I know you already have it inlined; your call.

pkg/spanconfig/spanconfigsqlwatcher/buffer.go, line 255 at r2 (raw file):

		// target ProtectedTimestampUpdates there is no deduplication possible.
		if prevUpdate.IsClusterUpdate() && curUpdate.IsTenantsUpdate() ||
			prevUpdate.IsTenantsUpdate() && curUpdate.IsClusterUpdate() {

Can we ever have prevUpdate.IsClusterUpdate() and curUpdate.IsTenantUpdate?

pkg/spanconfig/spanconfigsqlwatcher/buffer.go, line 266 at r2 (raw file):

		}

		// If the previous buffered event, and the current event are both tenant

nit: move this at the top of the loop considering we sort tenant targets before cluster targets?

pkg/spanconfig/spanconfigsqlwatcher/sqlwatcher.go, line 374 at r2 (raw file):

		case *ptpb.Target_SchemaObjects:
			// For PTS records with schema object targets, unwrap the descriptor IDs,
			// and emit them as descriptor SQLUpdates. This allows for the deduplication

Coming back to this after reading your tests for the buffer, hijacking PTS updates on descriptor IDs at this level worked out well 💯

pkg/spanconfig/spanconfigsqlwatcher/buffer_test.go, line 109 at r2 (raw file):

	buffer.advance(descriptorsRangefeed, ts(10))
	require.NoError(t, err)
	buffer.advance(protectedTimestampRangefeed, ts(10))

nit: here, and below, instead of moving descriptors/pts rangefeeds in lockstep you could make the test case more interesting by choosing different values and making sure the lowest of the 3 is the combined frontier timestamp.

adityamaru

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

pkg/spanconfig/spanconfigsqlwatcher/buffer.go, line 109 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: do you think it's worth capturing the sort order here? I know you already have it inlined; your call.

Heh, I did have it here for a second and then moved it back inline cause it felt more natural to highlight to the reader that this sort.Sort is actually more involved than meets the eye.

pkg/spanconfig/spanconfigsqlwatcher/buffer.go, line 255 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Can we ever have prevUpdate.IsClusterUpdate() and curUpdate.IsTenantUpdate?

I refactored this to Irfan's suggestion above. You're right I don't think we could have in the older version of the code.

pkg/spanconfig/spanconfigsqlwatcher/buffer.go, line 266 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: move this at the top of the loop considering we sort tenant targets before cluster targets?

refactored to irfan's suggestion but also moveed this clause higher.

pkg/spanconfig/spanconfigsqlwatcher/buffer_test.go, line 109 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: here, and below, instead of moving descriptors/pts rangefeeds in lockstep you could make the test case more interesting by choosing different values and making sure the lowest of the 3 is the combined frontier timestamp.

staggered the timestamps as suggested.

adityamaru · 2022-02-03T03:42:30Z

Thanks for all the review on this!

bors r=irfansharif,arulajmani

craig · 2022-02-03T04:43:06Z

Build failed (retrying...):

GitHub CI (Cockroach)

otan · 2022-02-03T05:02:00Z

[04:31:38][run] pkg/spanconfig/spanconfigsqlwatcher/sqlwatcher.go:406:21: not enough arguments in call to rangefeed.WithDiff
[04:31:38][run] 	have ()
[04:31:38][run] 	want (bool)

merge skew :(

(yay bors!!!)

otan · 2022-02-03T05:02:08Z

bors r-

craig · 2022-02-03T05:02:09Z

Canceled.

…updates This change sets up a rangefeed on the `system.pts_records` table, and adds this to the set of rangefeeds managed by the SQLWatcher. We switch the unit emitted by the SQLWatcher to a union type called `SQLUpdate` which captures either an update to a descriptor or to a protected timestamp target. The zones and descriptor table rangefeeds will continue to emit descriptor updates. The pts rangefeed will emit descriptor updates for records that target schema objects, and pts target updates for records that target cluster or tenants. The SQLWatcher continues to dedup the SQLUpdates that it emits, so as to ensure there is only one SQLUpdate per descriptor ID, and one SQLUpdate per cluster or tenant target. The reconciler registers a handler that is invoked everytime a batch of SQLUpdates are emitted by the SQLWatcher. There is change in semantics in this part of the code, a future PR will teach the reconciler to parse the pts target SQLUpdates, and in turn instruct the SQLTranslator to generate the appropriate `SystemSpanConfigs`. Note, this file moves the sqlwatcher tests into a ccl package so as to be able to run backup statements to test the rangefeed on the pts table. Informs: cockroachdb#73727 Release note: None

adityamaru · 2022-02-03T16:22:02Z

CI failure is #75960.

bors r+

craig · 2022-02-03T18:59:33Z

Build succeeded:

GitHub CI (Cockroach)

adityamaru requested review from arulajmani and irfansharif January 19, 2022 03:23

adityamaru mentioned this pull request Jan 25, 2022

protectedts: rework protected timestamp storage for multi-tenant model #73727

Closed

23 tasks

adityamaru force-pushed the pts-sql-watcher branch 2 times, most recently from 9c5b161 to 782edc6 Compare January 27, 2022 02:59

adityamaru changed the title ~~[WIP,DNM] spanconfigsql{watcher,reconciler}: setup SQLWatcher to watch for pts updates~~ spanconfigsql{watcher,reconciler}: setup SQLWatcher to watch for pts updates Jan 27, 2022

adityamaru marked this pull request as ready for review January 27, 2022 03:00

adityamaru requested a review from a team as a code owner January 27, 2022 03:00

adityamaru requested a review from ajwerner January 27, 2022 03:00

adityamaru force-pushed the pts-sql-watcher branch 3 times, most recently from bb0dd79 to 16b1e7d Compare January 28, 2022 16:16

adityamaru force-pushed the pts-sql-watcher branch from 16b1e7d to 2f9b924 Compare January 28, 2022 16:21

irfansharif reviewed Jan 28, 2022

View reviewed changes

adityamaru force-pushed the pts-sql-watcher branch from 2f9b924 to d393536 Compare February 1, 2022 14:18

blathers-crl bot requested a review from irfansharif February 1, 2022 14:18

adityamaru force-pushed the pts-sql-watcher branch 2 times, most recently from 9751b37 to d89104e Compare February 1, 2022 23:45

adityamaru force-pushed the pts-sql-watcher branch from d89104e to e0ce909 Compare February 1, 2022 23:52

irfansharif approved these changes Feb 2, 2022

View reviewed changes

arulajmani approved these changes Feb 2, 2022

View reviewed changes

adityamaru force-pushed the pts-sql-watcher branch from e0ce909 to 9a9a965 Compare February 2, 2022 23:06

adityamaru commented Feb 2, 2022

View reviewed changes

adityamaru force-pushed the pts-sql-watcher branch from 9a9a965 to 9bd8a1a Compare February 3, 2022 00:05

adityamaru force-pushed the pts-sql-watcher branch from 9bd8a1a to 63a248c Compare February 3, 2022 14:30

craig bot merged commit 0b16926 into cockroachdb:master Feb 3, 2022

adityamaru deleted the pts-sql-watcher branch February 3, 2022 19:00

irfansharif mentioned this pull request Apr 20, 2022

spanconfig: assertion failure in sqlconfigwatcher.combine #75831

Closed

Conversation

adityamaru commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Jan 19, 2022

Uh oh!

adityamaru commented Jan 19, 2022

Uh oh!

adityamaru commented Jan 27, 2022

Uh oh!

adityamaru commented Jan 28, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adityamaru commented Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

irfansharif left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arulajmani left a comment

Choose a reason for hiding this comment

Uh oh!

adityamaru left a comment

Choose a reason for hiding this comment

Uh oh!

adityamaru commented Feb 3, 2022

Uh oh!

craig bot commented Feb 3, 2022

Uh oh!

otan commented Feb 3, 2022

Uh oh!

otan commented Feb 3, 2022

Uh oh!

craig bot commented Feb 3, 2022

Uh oh!

adityamaru commented Feb 3, 2022

Uh oh!

craig bot commented Feb 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adityamaru commented Jan 19, 2022 •

edited

Loading

adityamaru commented Feb 1, 2022 •

edited

Loading