Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

importccl: re-enable job control tests #31556

Merged
merged 1 commit into from
Oct 22, 2018
Merged

importccl: re-enable job control tests #31556

merged 1 commit into from
Oct 22, 2018

Conversation

maddyblue
Copy link
Contributor

@maddyblue maddyblue commented Oct 17, 2018

I tracked down the problem. It was that after the CANCEL JOB was issued,
sometimes the 2nd stage of the IMPORT (the shuffle) would have started,
and sometimes it wouldn't have. If it did not start then RunJob would
block forever trying to send on the allowResponse chan. Fix this by
making a draining go routine after the first block.

Closes #24623
Closes #24658

Release note: None

@maddyblue maddyblue requested review from dt and a team October 17, 2018 19:39
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@maddyblue
Copy link
Contributor Author

I guess this is still failing. Ugggggggh. Running more tests on my laptop.

@maddyblue
Copy link
Contributor Author

Updated with a real fix. PTAL.

Copy link
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/ccl/importccl/import_stmt_test.go, line 1536 at r1 (raw file):

		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			if r.Method == "GET" {
				// The following code correctly (I hope) handles both the case where, after

No need to leave the I hope in there. Either it does (in which case you're unlikely to ever remove the I hope) or it doesn't in which case another failure will occur and you'll work on another fix.


pkg/ccl/importccl/import_stmt_test.go, line 1548 at r1 (raw file):

					for range allowResponse {
					}
				}()

Probably doesn't matter, but I think this more naturally belongs inside of the once.Do.

Copy link
Contributor Author

@maddyblue maddyblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/ccl/importccl/import_stmt_test.go, line 1536 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

No need to leave the I hope in there. Either it does (in which case you're unlikely to ever remove the I hope) or it doesn't in which case another failure will occur and you'll work on another fix.

Done.


pkg/ccl/importccl/import_stmt_test.go, line 1548 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Probably doesn't matter, but I think this more naturally belongs inside of the once.Do.

Done.

I tracked down the problem. It was that after the CANCEL JOB was issued,
sometimes the 2nd stage of the IMPORT (the shuffle) would have started,
and sometimes it wouldn't have. If it did not start then RunJob would
block forever trying to send on the allowResponse chan. Fix this by
making a draining go routine after the first block.

Closes #24623
Closes #24658

Release note: None
@maddyblue
Copy link
Contributor Author

bors r+

craig bot pushed a commit that referenced this pull request Oct 22, 2018
31556: importccl: re-enable job control tests r=mjibson a=mjibson

I tracked down the problem. It was that after the CANCEL JOB was issued,
sometimes the 2nd stage of the IMPORT (the shuffle) would have started,
and sometimes it wouldn't have. If it did not start then RunJob would
block forever trying to send on the allowResponse chan. Fix this by
making a draining go routine after the first block.

Closes #24623
Closes #24658

Release note: None

31627: storage: remove spurious call to maybeInlineSideloadedRaftCommand r=nvanbenschoten,benesch a=tschottdorf

Entries are "thinned" only when passed to `r.append()` (i.e. written to
disk) and they are always returned "fat" from `Entries()` (i.e. Raft's way
to get entries from disk). Consequently Raft never sees thin entries and
won't ask us to commit them.

Touches #31618.

Release note: None

31695: bitarray: don't allow FromEncodingParts to return invalid bit array r=knz a=benesch

It is invalid for a bit array's lastBitsUsed field to be greater than
64. The FromEncodingParts function, however, would happily construct
an invalid bitarray if given a too-large lastBitsUsed value. Teach the
FromEncodingParts to return an error instead.

This presented as a panic when attempting to pretty-print a key with a
bitarray whose lastBitsUsed encoded value was 65. Such a key can be
created when calling PrefixEnd on a key with a bitarray whose
lastBitsUsed value is 64. By returning an error instead, the
pretty-printing code will try again after calling UndoPrefixEnd and be
able to print the key.

Fix #31115.

Release note: None

31697: partitionccl: deflake TestDropIndexWithZoneConfigCCL r=danhhz,eriktrinh a=benesch

A particularly adversarial goroutine schedule can cause this test to
observe the moment in time where the data is dropped but the zone config
is not. Deflake by retrying the check for the dropped zone config until
it succeeds (or times out).

Fix #31678.

Release note: None

Co-authored-by: Matt Jibson <matt.jibson@gmail.com>
Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com>
Co-authored-by: Nikhil Benesch <nikhil.benesch@gmail.com>
@craig
Copy link
Contributor

craig bot commented Oct 22, 2018

Build succeeded

  • GitHub CI (Cockroach)

@craig craig bot merged commit c6095ad into cockroachdb:master Oct 22, 2018
@maddyblue maddyblue deleted the enable-job-control branch October 23, 2018 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants