Skip to content

backupccl: add missing context cancel checks in gen split scatter processor#96529

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
rhu713:gssp-ctx-cancel
Feb 8, 2023
Merged

backupccl: add missing context cancel checks in gen split scatter processor#96529
craig[bot] merged 1 commit intocockroachdb:masterfrom
rhu713:gssp-ctx-cancel

Conversation

@rhu713
Copy link
Copy Markdown
Contributor

@rhu713 rhu713 commented Feb 3, 2023

Add the rest of the missing context cancel checks in restore's generativeSplitAndScatterProcessor. Add a red/green test to show that runGenerativeSplitAndScatter is interrupted if its supplied context is canceled.

Fixes: #95257

Release note: None

@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Feb 3, 2023

This change is Reviewable

@rhu713 rhu713 requested a review from adityamaru February 3, 2023 21:59
@rhu713 rhu713 marked this pull request as ready for review February 3, 2023 22:00
@rhu713 rhu713 requested review from a team as code owners February 3, 2023 22:00
@rhu713 rhu713 requested a review from michae2 February 3, 2023 22:00
@healthy-pod
Copy link
Copy Markdown
Contributor

healthy-pod commented Feb 7, 2023

@rhu713 / @adityamaru Does this race condition sound legit? I understand Bazel Extended CI passed but I am running testrace on this PR on a different GCP machine type and get that. Trying to understand if it's an issue with the new machine or if this is a legit data race that wasn't detected on the current TeamCity agent machine.

@rhu713 rhu713 force-pushed the gssp-ctx-cancel branch 2 times, most recently from 8d03729 to 6e66631 Compare February 7, 2023 20:23
backupccl: add missing context cancel checks in gen split scatter pro…
d91fc4b 
…cessor

Add the rest of the missing context cancel checks in restore's
generativeSplitAndScatterProcessor. Add a red/green test to show that
runGenerativeSplitAndScatter is interrupted if its supplied context is
canceled.

Fixes: cockroachdb#95257

Release note: None
@rhu713
Copy link
Copy Markdown
Contributor Author

rhu713 commented Feb 8, 2023

@rhu713 / @adityamaru Does this race condition sound legit?

I think it looks legit but I can't reproduce it on my machine. I've added a mutex to the read of the variable which hopefully fixes the race.

michae2
michae2 approved these changes Feb 8, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @rhu713)

@rhu713
Copy link
Copy Markdown
Contributor Author

rhu713 commented Feb 8, 2023

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Feb 8, 2023

Build succeeded:

@craig craig bot merged commit dbfc71b into cockroachdb:master Feb 8, 2023
@rhu713 rhu713 mentioned this pull request Feb 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michae2 michae2 michae2 approved these changes

+1 more reviewer

@adityamaru adityamaru adityamaru approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

roachtest: restore/tpce/8TB/aws/nodes=10/cpus=8 failed

5 participants

@rhu713 @cockroach-teamcity @healthy-pod @adityamaru @michae2