bulkio: Correctly group producer/consumers when importing data

[miretskiy]

Fixes #49989

Correctly link data import producer and consumer when importing data.
Using separate groups for producer and consumer could lead to a situation
where consumer exits (due to an error, or explicit context cancellation), while
producer does not realize that, leading to a deadlock.

Release notes (bug fix): Correctly link producer/consumer during data
import in order to handle errors correctly.

[cockroach-teamcity]

This change is 

[nvanbenschoten]

This , but the group/producerGroup split looks deliberate enough that it warrants either a) some archeology and an explanation on this PR about why it was built that way to begin with and why the split is not actually needed, or b) a review from @spaskob, who added it in 736e88d, or c) preferably both.

Thanks again for the quick fix!

Also, I meant to ask on the other PR - you're planning on backporting both of these to release-20.1, right?

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

---

pkg/ccl/importccl/import_processor_test.go, line 442 at r1 (raw file):

}

func TestImportHandlesDuplicateKVs(t *testing.T) {

Nice test.

[pbardea]

This also LGTM, but I agree with Nathan's proposal that this would probably benefit from a quick look from @spaskob to ensure that everything is indeed what we expect.

[spaskob]

[miretskiy]

Thanks for the reviews. I did do some digging. From what I can tell, this was simply a refactoring move, trying to perturb the code the least. There used to be input finished call, which was responsible for closing kvch. The refactor removed that.

It is simply no correct to have 2 groups. The issue with deadlock was that when one of the workers in group B exits (ingetstKV returns error), the other worker responsible for
waiting for the import to finish, would never finish since workers in Group A try to write to channel that nobody reads from. There is no communication (vis-a-vis errors, and/or cancellation) between group B and A.

[spaskob]

Yes agreed, thanks for fixing my mess!

…

On Tue, Jun 9, 2020 at 11:48 AM Yevgeniy Miretskiy ***@***.***> wrote: Thanks for the reviews. I did do some digging. From what I can tell, this was simply a refactoring move, trying to perturb the code the least. There used to be input finished call, which was responsible for closing kvch. The refactor removed that. It is simply no correct to have 2 groups. The issue with deadlock was that when one of the workers in group B exits (ingetstKV returns error), the other worker responsible for waiting for the import to finish, would never finish since workers in Group A try to write to channel that nobody reads from. There is no communication (vis-a-vis errors, and/or cancellation) between group B and A. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#49995 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANL6324XLBO4HWQSZKCHFLRVZKTDANCNFSM4NY3VI7Q> .

[miretskiy]

Also, CI failures were due to the stress test because I also need the fix already submitted (to parallel producer). The newly added test also triggers the other deadlock bug #49977

[miretskiy]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)