backupccl: distribute RESTORE work using DistSQL #51959
Conversation
|
This change is |
|
This should be ready for a review, but shouldn't be merged until #51607 is merged and the scatter processor can actually find the destination of the span it imported. |
pbardea
force-pushed
the
restore-planning
branch
from
b864463
to
94e2857
Compare
pkg/ccl/backupccl/restore_job.go
Outdated
| }{ | ||
| highWaterMark: -1, | ||
| } | ||
|
|
||
| res := RowCount{} |
There was a problem hiding this comment.
It took me a minute to figure out why this wasn't in the mutex-protected set above. I think it is OK to not be, but it might confuse future readers too without a comment (or we could just put it back under the mu, since it should be ~uncontended anyway?)
There was a problem hiding this comment.
Done - also moved back requestsCompleted for similar reasons. I also am now asserting that the dataspan we're marking as done is the one that has the same progress idx.
pbardea
force-pushed
the
restore-planning
branch
3 times, most recently
from
a21635b
to
c634534
Compare
There was a problem hiding this comment.
LGTM
Reviewed 3 of 4 files at r1, 3 of 4 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @dt, @pbardea, and @yuzefovich)
pkg/ccl/backupccl/restore_processor_planning.go, line 17 at r2 (raw file):
"github.com/cockroachdb/cockroach/pkg/kv" roachpb "github.com/cockroachdb/cockroach/pkg/roachpb"
super nit: are these roachpb and hlc aliases necessary?
pkg/ccl/backupccl/restore_processor_planning.go, line 71 at r2 (raw file):
if len(splitAndScatterSpecs) == 0 { close(progCh)
nit: we probably can defer this close. I'd also mention in the comment to the method that progCh is closed.
pbardea
force-pushed
the
restore-planning
branch
from
c634534
to
5e92de3
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/backupccl/restore_processor_planning.go, line 17 at r2 (raw file):
Previously, yuzefovich wrote…
super nit: are these
roachpbandhlcaliases necessary?
Nope, those were somehow auto-generated. Thanks for catching!
pkg/ccl/backupccl/restore_processor_planning.go, line 71 at r2 (raw file):
Previously, yuzefovich wrote…
nit: we probably can defer this close. I'd also mention in the comment to the method that
progChis closed.
Done and done.
This commit replaces restore's old method of coordinating work on a single node to instead use DistSQL. It creates a 2 stage DistSQL flow. The first stage assigns chunks to SplitAndScatter processors which forward spans that they scattered to the second stage made up of the RestoreData processors which ingest the data. The SplitAndScatter processors attempt to send the work to the RestoreData colocated with the leaseholder of the span after the scattering. Release note: None.
pbardea
force-pushed
the
restore-planning
branch
from
5e92de3
to
0fe825a
Compare
|
TFTRs! |
|
Build succeeded: |
This commit replaces restore's old method of coordinating work on a
single node to instead use DistSQL. It creates a 2 stage DistSQL flow.
The first stage assigns chunks to SplitAndScatter processors which
forward spans that they scattered to the second stage made up of the
RestoreData processors which ingest the data. The SplitAndScatter
processors attempt to send the work to the RestoreData colocated with
the leaseholder of the span after the scattering.
Release note: None.