roachtest: better merge testing in clearrange #29646

tbg · 2018-09-06T15:53:22Z

Unfortunately, the method to determine the range count is quite slow since
crdb_internal.ranges internally sends an RPC for each range to determine
the leaseholder.

Anecdotally, I've seen ~25% of the merges completed after less than 15
minutes. I know that it's slowing down over time, but @benesch will fix
that.

Also throws in aggressive consistency checks so that when something goes
out of sync, we find out right there.

Release note: None

cockroach-teamcity · 2018-09-06T15:53:30Z

This change is

tbg · 2018-09-06T16:07:50Z

Guess what, folks? We got our repro right away!

cc #29252.

E180906 16:06:58.941627 103825 storage/replica_consistency.go:94  [merge,n8,s8,r9836/3:/Table/53/1/31{599388-753605}] replica (n9,s9):2 is inconsistent: expected checksum ba45b571796dd4ddd8d54087011abab0e0f91cb0d57052b642f384cc70cc98416c093249ef9edaa0b56d531e66d74480009fa1770bdc71b043d969e00f1a771a, got dd16df53ab4308b10934b5eb78b9073c2c6d20ea37e6682e12d75394c26767177536539f0498aef999b9bd30390b3a86bea12a1c3e9949cf6334758af5da7840
--- leaseholder
+++ follower
+0.000000000,0 /Local/Range/Table/53/1/31627334/Transaction/"5a9a2ffe-5ce0-480d-b83d-eaa8b3479155"
+    ts:<zero>
+    value:"\x12\x04\b\x00\x10\x00\x18\x00 \x00(\x002\x81\x01P\xd0$\xdd\x03\n6\n\x10Z\x9a/\xfe\\\xe0H\r\xb8=ꨳG\x91U\x1a\x10\x01k\x12\xbd\x89xf9\x01\xe2\x98F\x00\x01rdsc*\n\b\xba\xb2\xe3Б\xe7\xf6\xa8\x150\xbb\x99\x1d8\x01\x12\x05merge*\n\b\x95\xd8\xfaБ\xe7\xf6\xa8\x152\n\b\xba\xb2\xe3Б\xe7\xf6\xa8\x15:\n\b\xba\xb2\xe3Б\xe7\xf6\xa8\x15B\x0e\b\x03\x12\n\b\xba\xb2\xe3Б\xe7\xf6\xa8\x15H\x01r\x00z\x00\x80\x01\x01"
+    raw mvcc_key/value: 016b12bd89f901e29846000174786e2d5a9a2ffe5ce0480db83deaa8b347915500 12040800100018002000280032810150d024dd030a360a105a9a2ffe5ce0480db83deaa8b34791551a10016b12bd89f901e298460001726473632a0a08bab2e3d091e7f6a81530bb991d380112056d657267652a0a0895d8fad091e7f6a815320a08bab2e3d091e7f6a8153a0a08bab2e3d091e7f6a815420e0803120a08bab2e3d091e7f6a815480172007a00800101
E180906 16:06:58.941858 103825 util/log/crash_reporting.go:477  [merge,n8,s8,r9836/3:/Table/53/1/31{599388-753605}] Reported as error 875dfcad015e4c82845f68a1293eb893

benesch

Reviewed 3 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/cmd/roachtest/clearrange.go, line 97 at r2 (raw file):

					}
				} else {
					startHex = "bd" // extremely likely to be the right thing (b'\275').

loooool

pkg/cmd/roachtest/clearrange.go, line 110 at r2 (raw file):

					return n
				}
			}()

I don't understand why this needs the outer layer of closuring. Isn't this equivalent?

conn := c.Conn(ctx, 1)
defer conn.Close()

var startHex string
// NB: set this to false to save yourself some time during development. Selecting
// from crdb_internal.ranges is very slow because it contacts all of the leaseholders.
// You may actually want to run a version of cockroach that doesn't do that because
// it'll still slow you down every time the method returned below is called.
if true {
	if err := conn.QueryRow(
		`SELECT to_hex(start_key) FROM crdb_internal.ranges WHERE "database" = 'bank' AND "table" = 'bank' ORDER BY start_key ASC LIMIT 1`,
	).Scan(&startHex); err != nil {
		t.Fatal(err)
	}
} else {
	startHex = "bd" // extremely likely to be the right thing (b'\275').
}

numBankRanges := func() int {
	var n int
	if err := conn.QueryRow(
		`SELECT COUNT(*) FROM crdb_internal.ranges WHERE substr(to_hex(start_key), 1, length($1::string)) = $1`, startHex,
	).Scan(&n); err != nil {
		t.Fatal(err)
	}
	return n
}

We saw a consistency failure in cockroachdb#29252 that would've been much more useful had it occurred close to the time around which the inconsistency must have been introduced. Instead of leaving it to chance, add a switch that runs aggressive checks in (roach) tests that want them such as the clearrange test. Release note: None

tbg

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/cmd/roachtest/clearrange.go, line 110 at r2 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I don't understand why this needs the outer layer of closuring. Isn't this equivalent?

conn := c.Conn(ctx, 1)
defer conn.Close()

var startHex string
// NB: set this to false to save yourself some time during development. Selecting
// from crdb_internal.ranges is very slow because it contacts all of the leaseholders.
// You may actually want to run a version of cockroach that doesn't do that because
// it'll still slow you down every time the method returned below is called.
if true {
	if err := conn.QueryRow(
		`SELECT to_hex(start_key) FROM crdb_internal.ranges WHERE "database" = 'bank' AND "table" = 'bank' ORDER BY start_key ASC LIMIT 1`,
	).Scan(&startHex); err != nil {
		t.Fatal(err)
	}
} else {
	startHex = "bd" // extremely likely to be the right thing (b'\275').
}

numBankRanges := func() int {
	var n int
	if err := conn.QueryRow(
		`SELECT COUNT(*) FROM crdb_internal.ranges WHERE substr(to_hex(start_key), 1, length($1::string)) = $1`, startHex,
	).Scan(&n); err != nil {
		t.Fatal(err)
	}
	return n
}

Sure, that works, but I always find it a bit unsavory. Also, note that we're calling SET statement_timeout below, though you get lucky and it's on a different conn. Might just be my preference, but I like having stuff defined locally and not floating around as a quasiglobal. Makes my brain melt less.

tbg

This PR is now @benesch's to probably close and reopen under his name.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

@benesch

Unfortunately, the method to determine the range count is quite slow since crdb_internal.ranges internally sends an RPC for each range to determine the leaseholder. Anecdotally, I've seen ~25% of the merges completed after less than 15 minutes. I know that it's slowing down over time, but @benesch will fix that. Also throws in aggressive consistency checks so that when something goes out of sync, we find out right there. Release note: None

benesch

This PR is now @benesch's to probably close and reopen under his name.

I'm actually going to merge it basically as-is. The one change I made I noted below; doesn't seem worthy of another review cycle. Merging as soon as I verify this passes with the fix from #29677.

Reviewed 1 of 1 files at r3, 1 of 1 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/cmd/roachtest/clearrange.go, line 110 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Sure, that works, but I always find it a bit unsavory. Also, note that we're calling SET statement_timeout below, though you get lucky and it's on a different conn. Might just be my preference, but I like having stuff defined locally and not floating around as a quasiglobal. Makes my brain melt less.

Ack. Returning a closure from a closure ranks as more confusing than a stray state variable to me, but to each his own. I'm going to leave as is.

pkg/cmd/roachtest/clearrange.go, line 125 at r4 (raw file):

					return err
				}

Added this to speed up merges.

benesch · 2018-09-07T00:59:05Z

All is well.

bors r+

craig · 2018-09-07T01:00:24Z

Canceled (will resume)

benesch · 2018-09-07T01:06:20Z

And now Bors is stuck. Great.

bors r+

craig · 2018-09-07T01:06:21Z

Not awaiting review

benesch · 2018-09-07T01:06:37Z

bors r-

craig · 2018-09-07T01:06:38Z

Canceled

benesch · 2018-09-07T01:07:46Z

bors r+

@benesch

29646: roachtest: better merge testing in clearrange r=benesch a=tschottdorf Unfortunately, the method to determine the range count is quite slow since crdb_internal.ranges internally sends an RPC for each range to determine the leaseholder. Anecdotally, I've seen ~25% of the merges completed after less than 15 minutes. I know that it's slowing down over time, but @benesch will fix that. Also throws in aggressive consistency checks so that when something goes out of sync, we find out right there. Release note: None 29677: storage: preserve consistency when applying widening preemptive snapshots r=benesch a=benesch Merges can cause preemptive snapshots that widen existing replicas. For example, consider the following sequence of events: 1. A replica of range A is removed from store S, but is not garbage collected. 2. Range A subsumes its right neighbor B. 3. Range A is re-added to store S. In step 3, S will receive a preemptive snapshot for A that requires widening its existing replica, thanks to the intervening merge. Problematically, the code to check whether this widening was possible, in Store.canApplySnapshotLocked, was incorrectly mutating the range descriptor in the snapshot header! Applying the snapshot would then fail to clear all of the data from the old incarnation of the replica, since the bounds on the range deletion tombstone were wrong. This often resulted in replica inconsistency. Plus, the in-memory copy of the range descriptor would be incorrect until the next descriptor update--though this usually happened quickly, as the replica would apply the change replicas command, which updates the descriptor, soon after applying the preemptive snapshot. To fix the problem, teach Store.canApplySnapshotLocked to make a copy of the range descriptor before it mutates it. To prevent regressions, add an assertion that a range's start key is never changed to the descriptor update path. With this assertion in place, but without the fix itself, TestStoreRangeMergeReadoptedLHSFollower reliably fails. Fixes #29252. Release note: None Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com> Co-authored-by: Nikhil Benesch <nikhil.benesch@gmail.com>

craig · 2018-09-07T01:26:38Z

Build succeeded

GitHub CI (Cockroach)

tbg requested a review from a team September 6, 2018 15:53

tbg requested a review from benesch September 6, 2018 15:53

benesch approved these changes Sep 6, 2018

View reviewed changes

tbg force-pushed the roachtest/merges branch from a2dd2b6 to 2a6c8e0 Compare September 6, 2018 16:20

tbg commented Sep 6, 2018

View reviewed changes

benesch force-pushed the roachtest/merges branch from 2a6c8e0 to 5bd9941 Compare September 6, 2018 21:54

benesch approved these changes Sep 6, 2018

View reviewed changes

craig bot merged commit 5bd9941 into cockroachdb:master Sep 7, 2018

benesch mentioned this pull request Sep 9, 2018

roachtest: clearrange does not verify expected number of merged ranges #29131

Closed

tbg deleted the roachtest/merges branch September 21, 2018 11:33

tbg mentioned this pull request Sep 21, 2018

backport-2.1: assorted testing-related backports #30503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roachtest: better merge testing in clearrange #29646

roachtest: better merge testing in clearrange #29646

tbg commented Sep 6, 2018

cockroach-teamcity commented Sep 6, 2018

tbg commented Sep 6, 2018 •

edited

Loading

benesch left a comment

tbg left a comment

tbg left a comment

benesch left a comment

benesch commented Sep 7, 2018

craig bot commented Sep 7, 2018

benesch commented Sep 7, 2018

craig bot commented Sep 7, 2018

benesch commented Sep 7, 2018

craig bot commented Sep 7, 2018

benesch commented Sep 7, 2018

craig bot commented Sep 7, 2018

roachtest: better merge testing in clearrange #29646

roachtest: better merge testing in clearrange #29646

Conversation

tbg commented Sep 6, 2018

cockroach-teamcity commented Sep 6, 2018

tbg commented Sep 6, 2018 • edited Loading

benesch left a comment

Choose a reason for hiding this comment

tbg left a comment

Choose a reason for hiding this comment

tbg left a comment

Choose a reason for hiding this comment

benesch left a comment

Choose a reason for hiding this comment

benesch commented Sep 7, 2018

craig bot commented Sep 7, 2018

Canceled (will resume)

benesch commented Sep 7, 2018

craig bot commented Sep 7, 2018

benesch commented Sep 7, 2018

craig bot commented Sep 7, 2018

Canceled

benesch commented Sep 7, 2018

craig bot commented Sep 7, 2018

Build succeeded

tbg commented Sep 6, 2018 •

edited

Loading