-
Notifications
You must be signed in to change notification settings - Fork 4k
sql: add SST writer to distributed index backfill merge pipeline #158456
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
sql: add SST writer to distributed index backfill merge pipeline #158456
Conversation
1e32c62 to
82969e9
Compare
fqazi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one issue related to the error handling for progress.
@fqazi reviewed 18 of 18 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @jeffswenson, @kev-cao, and @mw5h)
pkg/sql/rowexec/indexbackfiller.go line 425 at r1 (raw file):
case <-tick.C: if err := pushProgress(); err != nil { return err
Will this also terminate the ingest thread? I worry that we will just stop tracking progress.
fqazi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spilchen Nice work!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @jeffswenson, @kev-cao, @mw5h, and @spilchen)
This change continues the integration of the distributed merge pipeline into the index backfill flow. When enabled, the backfiller writes its KV output batches to SSTs (backed by ExternalStorage) via a new SST sink. - Processor: Updated rowexec backfiller to route KV batches through the SST sink. - Job progress: Persist SST manifest metadata in job progress via IndexBackfillMapProgress. Informs: cockroachdb#158378 Epic: CRDB-48845 Release note: none
82969e9 to
b703953
Compare
spilchen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @fqazi, @jeffswenson, @kev-cao, and @mw5h)
pkg/sql/rowexec/indexbackfiller.go line 425 at r1 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Will this also terminate the ingest thread? I worry that we will just stop tracking progress.
Ah good point. I remember we have this pattern in other places. I'll log a warning and continue if the progress fails . Note: I don't expect this path to fail, that path only exists if we fail to marshal the SST manifests somehow.
fqazi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fqazi reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball, @jeffswenson, @kev-cao, and @mw5h)
This change continues the integration of the distributed merge pipeline into the index backfill flow. When enabled, the backfiller writes its KV output batches to SSTs (backed by ExternalStorage) via a new SST sink.
Informs: #158378
Epic: CRDB-48845
Release note: none