high level first pass

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

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 63 at r2 (raw file):

		eventMix: map[fsm.Event]int{
			// eventOpenTxn opens an UPSERT or DELETE transaction.
			eventOpenTxn{}: 10,

why split this up? (not objecting, i just don't see offhand why it was necessary)

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 67 at r2 (raw file):

			// eventFeedMessage reads a message from the feed, or if the state machine
			// thinks there will be no message available, it falls back to
			// eventTransact.

nit: eventTransact is gone

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 74 at r2 (raw file):

			// eventResume RESUMEs the changefeed.
			eventResume{}: 1,

ditto here, why split this up?

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 123 at r2 (raw file):

			}
		} else {
			if err := abort(fsm.Args{Ctx: ctx, Extended: ns}); err != nil {

abort is not quite the opposite of commit, we should add a rollback event too

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 429 at r2 (raw file):

	if ns.currentTestColumnCount >= ns.maxTestColumnCount {
		// Do nothing if the table already has the maximum allowed columns.

bummer, i wonder if we can avoid generating this event by adding more state. IIRC there's some crazy pattern matching stuff in the fsm library that helps with the transition explosion, but I don't remember if it would be useful here. if that doesn't work out then i agree it's probably not worth the extra state

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 441 at r2 (raw file):

	// Adding a column should trigger a full table scan.
	ns.db.QueryRow(`SELECT count(*) FROM foo`).Scan(&rows)
	ns.availableRows += rows

it should be twice this, right? the schema change itself does a kv backfill (which generates uninteresting rows at the changefeed level that we don't have a good way to filter) and then the changefeed runs a backfill after the schema change finishes

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 452 at r2 (raw file):

	}
	if ns.currentTestColumnCount == 0 {
		// Do nothing if the table has no test columns.

ditto unfortunate

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 471 at r2 (raw file):

	if ns.availableRows <= 0 {
		// No-op if there are no available rows to read.

if we're seeing this, it's because the availableRows accounting is wrong. we should probably look into that

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 529 at r2 (raw file):

	ns := a.Extended.(*nemeses)
	defer func() { ns.txn = nil }()
	return ns.txn.Rollback()

hmm, i don't think we want to do this change? it was intentionally aborting a txn without its knowledge before

pkg/ccl/changefeedccl/cdctest/validator.go, line 216 at r2 (raw file):

	// table and then apply the row update. We do it this way to ensure that
	// the set of non-null columns in the scratch table is equal to the set of
	// columns in the original table.

I don't quite follow, do you have an example?

pkg/ccl/changefeedccl/cdctest/validator.go, line 350 at r2 (raw file):

		// Ignore the fingerprint mismatch if there was an in-progress schema change job
		// on the table.
		// TODO(aayush): This is pretty hacky, how do we improve this?

it'd be nice to address this TODO before merge

Reviewed 1 of 1 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15, @ajwerner, and @danhhz)

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 291 at r2 (raw file):

func (eventFinished) Event()     {}

var compatibleStateTransitions = fsm.Pattern{

nit: just call this stateTransitions?

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 300 at r2 (raw file):

	stateRunning{Paused: fsm.False, TxnOpen: fsm.False}: {
		eventPause{}: {
			Next:   stateRunning{Paused: fsm.True, TxnOpen: fsm.False},

This comment is more for my own edification than asking for a change. I feel like we lack principle about which transitions we're making explicitly possible in the FSM vs checking inside the implementation. For example, we're keeping track of row counts and column counts inside the extended state and then changing behavior inside of actions based on that extended state.

Moving more things into the state machine itself is heavy-weight due to the transition explosion but not moving it into the state machine makes the logic more implicit and harder to understand.

I wonder if it would be good to have some sort of mechanism to create predicates over the extended state and use them as pre-conditions for transitions.

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 356 at r2 (raw file):

}

var txnStateTransitions = fsm.Compile(compatibleStateTransitions)

nit: I know this is the old name but I don't love it. What about compiledStateTransitions?

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 403 at r2 (raw file):

	if deleteID == noDeleteSentinel {
		if err := txn.QueryRow(
			`UPSERT INTO foo VALUES ((random() * $1)::int, cluster_logical_timestamp()::string) RETURNING id, ts`,

This is interesting to me. Doesn't it mean that all updates are unpushable? Is there a reason that the timestamp needs to be cluster_logical_timestamp()? Could it just be now()?

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 514 at r2 (raw file):

		// Don't error out if we got pushed, but don't increment availableRows no
		// matter what error was hit.
		if strings.Contains(err.Error(), `restart transaction`) {

Just out of curiosity I wonder if there's a more principled way to check if this is a serializable restart. Maybe by unwrapping the error into something which could hand you an error code? I have no idea. This comment is mostly just my own curiosity.

pkg/ccl/changefeedccl/cdctest/validator.go, line 350 at r2 (raw file):

It's honestly surprising to me (in a good way) that this works.

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

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 403 at r2 (raw file):

it appears i neglected to write this down. @tbg @nvanbenschoten do you remember why we did this? I feel like an obviously unnecessary use of cluster_logical_timestamp() would have come up in code review

pkg/ccl/changefeedccl/cdctest/validator.go, line 350 at r2 (raw file):

yeah, ditto. the workaround is incredibly clever

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

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 63 at r2 (raw file):

It seemed easier to reason about if we split transact into an event that simply opens a transaction and then events that committed or rolled-back the transaction. I'm not super tied to this idea but I'd argue its a bit cleaner this way.

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 74 at r2 (raw file):

This is just so I can avoid some code duplication. In nextEvent I'm trying to use the state transitions map to get all the "compatible" state transitions and then randomly selecting one among them by using weights from this eventMix map. Adding eventResume here allows me to do the same with pause/resume without separate logic.

pkg/ccl/changefeedccl/cdctest/validator.go, line 216 at r2 (raw file):

Imagine that we have 4 columns in foo and 4 (non-null) columns in fprint. Now there is a removeColumn event that drops one of the columns in foo. Now, the row updates that are streamed because of this will only contain 3 columns, but we can't simply UPSERT them into fprint because that won't nullify the 4th column. So, we instead do a transactional DELETE, followed by an UPSERT to apply every row update.

pkg/ccl/changefeedccl/cdctest/validator.go, line 350 at r2 (raw file):

We could go one step further here and make sure that the table in question is indeed foo, but that would be a bit redundant since this is the only table in this test cluster that undergoes schema changes.

I explored ways to perhaps look at foo's table descriptor somehow and check if it has pending mutations, but that seems even worse. The challenging part is that any approach we pick here must disambiguate false negatives and "real" failures.

Are there any other ideas I can explore?

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

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 63 at r2 (raw file):

wfm. I had this originally and for some reason switched it to this, but I can't remember why. As long as this doesn't cause any other compromised I prefer it to

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 74 at r2 (raw file):

👍

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 441 at r2 (raw file):

Oh! Yes you're right. That's really a bug in testFeed, so sounds like a good reminder/TODO to add to this comment.

pkg/ccl/changefeedccl/cdctest/validator.go, line 216 at r2 (raw file):

we can't UPSERT (id, ts, col1, col2, col3) VALUES (...)?

pkg/ccl/changefeedccl/cdctest/validator.go, line 350 at r2 (raw file):

I'm missing some background. Why do the fingerprints not match?

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

pkg/ccl/changefeedccl/cdctest/validator.go, line 216 at r2 (raw file):

So if fprint contains col1, col2, col3, col4 and we UPSERT (id, ts, col1, col2, col3) VALUES (...) this only performs an update, and doesn't nullify col4. This would cause the validation to fail, even though it shouldn't.

pkg/ccl/changefeedccl/cdctest/validator.go, line 350 at r2 (raw file):

Essentially, because of the fact that schema changes with backfill increment the table descriptor version 3 times (the last one happening after the backfill is "complete"). Lets say we're at table descriptor version 1, a schema change (with backfill) begins and it increments it to version 2, then to version 3 (but not to version 4, which it would do once the backfill is complete). If there is a resolved timestamp when the original table is at version 3, it would not contain the dropped (or added) column, but since the backfill has not finished yet, the fprint table still contains that about-to-be-dropped (or doesn't contain the newly added) column. So when we validate at this aforementioned resolved timestamp, we get a mismatch.

The "hack" here ignores the validation failure if it happened at a time where there was a pending schema change on the original table.

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

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 429 at r2 (raw file):

can we use fsm.Any somehow?

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 441 at r2 (raw file):

the testFeeds are intended to remove (key,ts) duplicates, but I was lazy and removed (key, value) duplicates. see the numerous // TODO(dan): Track duplicates more precisely in sinklessFeed/tableFeed. comments in TestChangefeedSchemaChangeAllowBackfill

pkg/ccl/changefeedccl/cdctest/validator.go, line 216 at r2 (raw file):

oh because fprint is a fixed number of columns, right. Then how about filling out all of col4...col10 with NULLs in the UPSERT?

pkg/ccl/changefeedccl/cdctest/validator.go, line 350 at r2 (raw file):

I'm worried about the hack because we lose coverage in a really tricky spot (during schema change backfill) so it'd be nice to address it if we can.

I'm still trying to wrap my head around why these don't match. If the original table is at version 3 and hasn't had the column dropped, shouldn't the fprint table also still have the column, and thus a matching fingerprint.

Side node: I'm incredibly surprised that the fingerprint code is actually resilient to the two tables having a different number of columns (as long as the one with extra columns has them filled with NULLs?). I did not intend that when writing it

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

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 429 at r2 (raw file):

We can make this work by providing variables in the event. For example we can include in eventAddColumn and eventRemoveColumn whether they will change the state.

For example:

type stateRunning struct {
    ...
    CanRemoveColumn fsm.Bool
    CanAddColumn fsm.Bool
}

type eventAddColumn struct {
    CanAddColumnAfter fsm.Bool
}

type eventRemoveColumn struct {
    CanRemoveColumnAfter fsm.Bool
}

Then in the transitions:

stateRunning{Paused: fsm.Var("paused"), TxnOpen: fsm.False, CanRemoveColumn: fsm.Var("can_remove"), CanAddColumn: fsm.True}: {
    eventAddColumn{CanAddColumnAfter: fsm.Var("can_add_after")}: { 
        Next:   stateRunning{
            Paused: fsm.Var("paused"), 
            TxnOpen: fsm.False, 
            CanRemoveColumn: fsm.True,
            CanAddColumn: fsm.Var("can_add_after"),
        },
        Action: logEvent(addColumn),
    } 
}

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

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 429 at r2 (raw file):

This works because we can know when we create the event whether we'll reach the threshold

Reviewed 1 of 1 files at r1, 1 of 2 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15, @ajwerner, @danhhz, and @tbg)

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 67 at r2 (raw file):

nit: eventOpenTxn or eventCommit

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 123 at r2 (raw file):

+1

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 403 at r2 (raw file):

I don't have any memory of this, but it does seem likely that this is to prevent the transaction from being auto-retried when being pushed.

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 429 at r2 (raw file):

Yeah, that would work. It still might be too heavyweight to justify though.

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 529 at r2 (raw file):

You mean TxnOpen = true? I think we would leave the transaction open.

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 76 at r3 (raw file):

			eventResume{}: 1,

			// eventCommit commits the outstanding transaction.

nit: do you mind grouping all of the transaction-related events together?

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 142 at r3 (raw file):

		return nil, err
	}
	// Now push everything to make sure the initial scan can complete, otherwise

Why were we able to get rid of this?

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 143 at r3 (raw file):

	for i := 0; i < ns.maxTestColumnCount; i++ {
		testCol := fmt.Sprintf(", test%d STRING DEFAULT NULL", i)
		createFprintStmtBuf.WriteString(testCol)

fmt.Fprintf

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 231 at r3 (raw file):

	}

	// Filter out `eventFinished` and restore at the end of the method.

Woah, this is super gross. If eventFinished has a weight of 0, do we even need this?

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 261 at r3 (raw file):

	}

	// If there are no available rows, openTxn or commit outstanding txn instead of

nit: move this into the if _, ok := event.(eventFeedMessage); ok { block and panic("unreachable") below the for loop.

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 264 at r3 (raw file):

	// reading.
	if ns.availableRows < 1 {
		if ns.txn == nil {

Can't we get this from the state? Seems cleaner.

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 273 at r3 (raw file):

type stateRunning struct {
	Paused  fsm.Bool

FeedPaused?

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 321 at r3 (raw file):

			Action: logEvent(noteFeedMessage),
		},
		eventSplit{}: {

I'm surprised we don't want to allow some of these while a transaction is ongoing.

pkg/ccl/changefeedccl/cdctest/validator.go, line 133 at r3 (raw file):

	failures   []string
	numColumns int

nit: we seem to keep failures []string separate, so much this up to the first group of fields.

pkg/sql/lease.go, line 989 at r2 (raw file):

		// because of its use of `singleflight.Group`. See issue #41780 for how this has
		// happened.
		newCtx, cancel := m.stopper.WithCancelOnQuiesce(logtags.WithTags(context.Background(), logtags.FromContext(ctx)))

WithCancelOnQuiesce is a pretty big footgun and I'm not sure that we even need it. Was the old context being canceled on quiesce?

pkg/util/fsm/fsm.go, line 145 at r3 (raw file):

}

// GetExpandedStateTransitions returns the underlying expanded pattern map.

Doesn't this belong as a method on Transitions?

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

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 231 at r3 (raw file):

Ah I didn't think of that. Done.

pkg/ccl/changefeedccl/cdctest/validator.go, line 350 at r2 (raw file):

If the original table is at version 3 and hasn't had the column dropped

At version 3, the original table will have had its last column dropped, but fprint would still have non-null values in that column. Those values for fprint would get nullified once the backfill is complete. Hence the mismatch.

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

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 67 at r2 (raw file):

Done.

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 123 at r2 (raw file):

Done.

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 429 at r2 (raw file):

I added this, I personally think this is pretty clean.

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 76 at r3 (raw file):

Done.

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

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 245 at r4 (raw file):

	}

	log.Infof(context.TODO(), "curstate: %#v", state)

remove this debug line?

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 269 at r4 (raw file):

			// If there are no available rows, openTxn or commit outstanding txn instead
			// of reading.
			if ns.availableRows < 1 {

I don't think that you should do this as it's pretty heavy but we could avoid all of this special casing if we made the state machine aware of whether or not there were available rows. Unfortunately to do that we'd need to track even more things like probably turn openTransaction into explicit delete and upsert events and only delete if you have rows and well ... you get the picture.

pkg/sql/lease.go, line 989 at r2 (raw file):

The old context was the clients context. That was the bug. Dan and I didn't have any suggestions on what a good timeout would be but if these calls hang (due to say the cluster being unavailable) we'll hang on shutdown. How do you see with cancel on quiesce as being problematic / a footgun here?

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

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 529 at r2 (raw file):

Done.

pkg/util/fsm/fsm.go, line 145 at r3 (raw file):

I wanted to avoid reCompileing the state transitions map. I restructured this bit so its a little bit cleaner. Let me know if this still seems lacking.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15, @danhhz, @nvanbenschoten, and @tbg)

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 479 at r5 (raw file):

I believe a schema change will block forever if a transaction is open.

modulo the one substantive comment about fingerprint(row.updated.Prev())

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15, @danhhz, @nvanbenschoten, and @tbg)

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 479 at r5 (raw file):

gotcha

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 63 at r6 (raw file):

		eventMix: map[fsm.Event]int{
			// We don't want `eventFinished` to ever be returned by `nextEvent` so we set
			// it's weight to 0.

nit: its

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 182 at r6 (raw file):

	}

	var txnOpenBeforeInitialScan bool

nit: slightly more obvious to put txnOpenBeforeInitialScan := false here

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 353 at r6 (raw file):

	},
	stateRunning{
		FeedPaused:      fsm.False,

any reason this one and the next can't work while feed is paused?

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 438 at r6 (raw file):

			Action: logEvent(abort),
		},
		eventRollback{}: {

group this with eventCommit

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 505 at r6 (raw file):

}

func push(a fsm.Args) error {

move this down right above/below abort

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 573 at r6 (raw file):

		return err
	}
	ns.availableRows += 2 * rows

comment plz, something out each row once for kv backfill once for changefeed backfill

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 594 at r6 (raw file):

		return err
	}
	ns.availableRows += 2 * rows

ditto comment here plz

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 639 at r6 (raw file):

}

func commit(a fsm.Args) error {

group this and rollback up under openTxn

pkg/ccl/changefeedccl/cdctest/validator.go, line 142 at r6 (raw file):

// exist before calling this constructor.
func NewFingerprintValidator(
	sqlDB *gosql.DB, origTable, fprintTable string, partitions []string, testColumns int,

testColumns is pretty vague. maybe something about variable table width? even if we don't end up with a better name, this deserves a mention in the godoc

pkg/ccl/changefeedccl/cdctest/validator.go, line 356 at r6 (raw file):

		}

		// If we have processed all the row updates belonging to a certain timestamp,

"If any updates have exactly equal timestamps, we have to apply them all before fingerprinting."

pkg/ccl/changefeedccl/cdctest/validator.go, line 360 at r6 (raw file):

		if len(v.buffer) == 0 || v.buffer[0].updated != row.updated {
			lastFingerprintedAt = row.updated
			if err := v.fingerprint(row.updated); err != nil {

I think you lost the fingerprint check for row.updated.Prev(). Without that, if we missed a row update we wouldn't catch it, right? Say k1 was written at t1, t2, t3. If we only output t1 and t3 would anything catch that right now?

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15, @danhhz, @nvanbenschoten, and @tbg)

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 63 at r6 (raw file):

Done.

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 182 at r6 (raw file):

Done.

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 353 at r6 (raw file):

I'd disabled it because it made each individual run of the nemeses last substantially longer (up to like 2mins in some cases, as opposed to ~5s before), and there didn't seem to be any reason why the feed being paused-unpaused would change anything wrt schema changes on the original table, maybe I'm wrong here?

I suspect the reason it takes this takes much longer is because it now stays in FeedPaused=true much longer (because eventAddColumn and eventRemoveColumn now contend with eventResume in that state). We could tune the event weights to fix this but I'm not sure this is even a problem as of now. What do you think?

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 438 at r6 (raw file):

Done.

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 505 at r6 (raw file):

Done.

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 573 at r6 (raw file):

Done

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 594 at r6 (raw file):

Done.

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 639 at r6 (raw file):

Done.

pkg/ccl/changefeedccl/cdctest/validator.go, line 142 at r6 (raw file):

Changed the name and added some explanation around what that parameter is. Let me know if it seems lacking in any way.

pkg/ccl/changefeedccl/cdctest/validator.go, line 356 at r6 (raw file):

Done

pkg/ccl/changefeedccl/cdctest/validator.go, line 360 at r6 (raw file):

Done

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15, @danhhz, @nvanbenschoten, and @tbg)

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 353 at r6 (raw file):
hmm yeah, i really do like it taking ~5s. 2m is far too long. what happens if you just give eventResume a really high weight? something like 100x what add/remove have? also totally fine just leaving this as a TODO

there didn't seem to be any reason why the feed being paused-unpaused would change anything wrt schema changes on the original table, maybe I'm wrong here?

all the bugs you've already found around schema changes were things we thought worked. my takeaway is that anything we can reasonably include in nemesis without diluting it, we should

pkg/ccl/changefeedccl/cdctest/validator.go, line 362 at r7 (raw file):

		// If we've processed all row updates belonging to the previous row's timestamp,
		// we fingerprint at `updated.Prev()` since want to catch cases where one or more

nit: since we want

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15, @ajwerner, @danhhz, @nvanbenschoten, and @tbg)

pkg/ccl/changefeedccl/cdctest/nemeses.go, line 353 at r6 (raw file):

It seems to be taking less time after increasing the weight for eventResume, which makes sense.

pkg/ccl/changefeedccl/cdctest/validator.go, line 362 at r7 (raw file):

Done.

pkg/sql/lease.go, line 989 at r2 (raw file):

Going to send out a new PR to address this.

Happy to see this going!

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @aayushshah15, @ajwerner, @danhhz, @nvanbenschoten, and @tbg)