*: disable merge queue in more tests #29235
Conversation
|
This change is |
|
If this LGTY, please send this through Bors to reduce the flakiness! |
pkg/internal/client/db.go
Outdated
| @@ -471,6 +471,7 @@ func (db *DB) AdminMerge(ctx context.Context, key interface{}) error { | |||
| // | |||
| // The keys can be either byte slices or a strings. | |||
| func (db *DB) AdminSplit(ctx context.Context, spanKey, splitKey interface{}) error { | |||
| return errors.New("manual split") | |||
There was a problem hiding this comment.
Hahaha, thanks. That's going to cause a test failure or fifty.
There was a problem hiding this comment.
bors r+
Reviewed 19 of 21 files at r1, 2 of 2 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (and 1 stale)
pkg/sql/distsql_physical_planner_test.go, line 99 at r2 (raw file):
func TestPlanningDuringSplitsAndMerges(t *testing.T) { defer leaktest.AfterTest(t)() t.Skip()
Add a comment. Or unskip this? Might be spurious.
Build failed |
|
bors r+ TestLint failed with the url checker :( |
|
I didn't look at the contents of this PR (yet), but be aware that there is also |
Build failed (retrying...) |
|
The current test failure looks like it's still caused by the merge queue: https://teamcity.cockroachdb.com/viewLog.html?buildId=869108&buildTypeId=Cockroach_UnitTests I'll merge the other PR. Let's smoke out the remaining flakes without messing up everyone else's day. |
Build failed |
I did, yep! TestImportPgDump is finding a real bug with merges, I think. I've plumbed in a DisableMergeQueue knob with a TODO for now. |
29230: server: deflake TestRapidRestarts r=benesch a=petermattis In go1.10 and earlier, it was not safe to call `http.ServeMux.ServeHTTP` concurrently with `http.ServeMux.Handle`. (This is fixed in go1.11). In the interim, provide our own safeServeMux wrapper that provides proper locking. Fixes #29227 Release note: None 29236: Revert "storage: enable the merge queue by default" r=tschottdorf a=benesch This reverts commit 98ca1d0. The merge queue will be reenabled once flaky tests are fixed. To reviewers: I'd much prefer to merge #29235. But if that gets stuck in code review or the flakiness reaches a breaking point, feel free to merge this instead. Co-authored-by: Peter Mattis <[email protected]> Co-authored-by: Nikhil Benesch <[email protected]>
|
bors r=tschottdorf |
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsql_physical_planner_test.go, line 99 at r2 (raw file):
Previously, tschottdorf (Tobias Schottdorf) wrote…
Add a comment. Or unskip this? Might be spurious.
Oops, didn't mean to commit. It's removed in the rev that's getting bors'd.
There was a problem hiding this comment.
Did you look at tests using kv.setupMultipleRanges()? E.g. TestTxnCoordSenderRetries should be in here but it's not.
Reviewable status:
There was a problem hiding this comment.
Also, did you check if any flake issues can be closed? "No" is a good enough answer, but I'd like to know for when looking at the ones assigned to me.
Reviewable status:
Agh, I thought those were all covered by |
There was only the one I linked in the PR description when I opened this last night. I'll do another sweep sometime this afternoon. |
Take a more aggressive approach to disabling the merge queue in tests. This commit disables the merge queue in all tests that send a manual AdminSplit request, unless those tests were explicitly verified to be safe to run with the merge queue on. Release note: None
|
This is contingent on #29324 going in first. |
|
bors r=tschottdorf |
1 similar comment
|
bors r=tschottdorf |
29235: *: disable merge queue in more tests r=tschottdorf a=benesch Take a more aggressive approach to disabling the merge queue in tests. This commit disables the merge queue in all tests that send a manual AdminSplit request, unless those tests were explicitly verified to be safe to run with the merge queue on. Release note: None Fix #29085. Fix #29242. Co-authored-by: Nikhil Benesch <[email protected]>
Build succeeded |
Take a more aggressive approach to disabling the merge queue in tests.
This commit disables the merge queue in all tests that send a manual
AdminSplit request, unless those tests were explicitly verified to be
safe to run with the merge queue on.
Release note: None
Fix #29085.
Fix #29242.