bulkio: Correctly group producer/consumers when importing data#49995
bulkio: Correctly group producer/consumers when importing data#49995craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
nvb
left a comment
There was a problem hiding this comment.
This 
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.
|
|
|
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 |
|
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>
.
|
Fixes cockroachdb#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.
|
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 |
|
bors r+ |
Build succeeded |
50089: release-20.1: bulkio: import no longer gets stuck due to errors encountered during import r=miretskiy a=miretskiy Backport: * 1/1 commits from "importccl: Correctly handle errors and cancellations during import." (#49979) * 1/1 commits from "bulkio: Correctly group producer/consumers when importing data" (#49995) Please see individual PRs for details. /cc @cockroachdb/release Co-authored-by: Yevgeniy Miretskiy <yevgeniy@cockroachlabs.com>
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.