importccl: Correctly handle errors and cancellations during import.

[miretskiy]

Fixes #49977

Parallel importer could get stuck due to a race between emitted
import batches and checking for context cancellation (either due to an
unforeseen error, or due to explicit context cancallation).

Fix the race condition, and add tests verifying correct behavior.

Release notes (bug fix): correctly handle import cancellation and errors.

[cockroach-teamcity]

This change is 

[nvanbenschoten]

thanks for the quick fix!

Reviewed 2 of 2 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @miretskiy and @pbardea)

---

pkg/ccl/importccl/read_import_base.go, line 580 at r1 (raw file):

}

// Flush flushes currently accumulated data.

nit: s/Flush/flush/

---

pkg/ccl/importccl/read_import_base_test.go, line 57 at r1 (raw file):

// nilDataProducer produces infinite stream of nulls.
// It implements importRowProducer

Missing period.

---

pkg/ccl/importccl/read_import_base_test.go, line 85 at r1 (raw file):

// errorReturningConsumer always returns a errConsumerAbortedError.
// it implements importRowConsumer

nit: s/it implemented importRowConsumer/It implemented importRowConsumer./

---

pkg/ccl/importccl/read_import_base_test.go, line 86 at r1 (raw file):

// errorReturningConsumer always returns a errConsumerAbortedError.
// it implements importRowConsumer
type errorReturningConsumer struct{}

It might be a little cleaner to add an err error field here so that we don't need the errConsumerAbortedError to be global. That also makes the consumer type a little more general.

---

pkg/ccl/importccl/read_import_base_test.go, line 97 at r1 (raw file):

// nilDataConsumer consumes and emits infinite stream of null.
// it implements importRowConsumer.

nit: s/it/It/

---

pkg/ccl/importccl/read_import_base_test.go, line 177 at r1 (raw file):

	require.NoError(t, ctxgroup.GroupWorkers(context.Background(), 100,
		func(_ context.Context, _ int) error {
			timeout := time.Millisecond * time.Duration(250+rand.Intn(1000))

750ms on average seems pretty long for a unit test. Does it need to be this long?

[miretskiy]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @pbardea)

---

pkg/ccl/importccl/read_import_base_test.go, line 177 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

750ms on average seems pretty long for a unit test. Does it need to be this long?

Nah... Lowered it to 250-500ms

[nvanbenschoten]

@pbardea should also give this a quick 👍 though.

Reviewed 2 of 2 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @pbardea)

[pbardea]

LGTM

[pbardea]

Ooops - sorry, I just noticed something: I think that we usually format release notes as:
Release note (bug fix): correctly handle import cancellation and errors. I think the formatting is important for the script that they run (- but I thought we had a linter to catch this?). It may also be useful to give the docs folks a quick description of what would happen if the race occurs.

[miretskiy]

done

…

On Mon, Jun 8, 2020 at 5:02 PM Paul Bardea ***@***.***> wrote: Ooops - sorry, I just noticed something: I think that we usually format release notes as: Release note (bug fix): correctly handle import cancellation and errors. I think the formatting is important for the script that they run (- but I thought we had a linter to catch this?). It may also be useful to give the docs folks a quick description of what would happen if the race occurs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#49979 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANA4FVFN2YEEQ37N6UMLX7DRVVGVXANCNFSM4NYWOTCA> .

[miretskiy]

bors r+

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)