storage/bulk,storagebase: add BulkAdderOptions struct #39496
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @dt)
pkg/sql/distsqlrun/bulk_row_writer.go, line 76 at r1 (raw file):
func (sp *bulkRowWriter) ingestLoop(ctx context.Context, kvCh chan []roachpb.KeyValue) error { writeTS := sp.spec.Table.CreateAsOfTime const bufferSize = 64 << 20
was increasing the flushSize to 32MB intentional (since you're not passing it in as an option)?
pkg/storage/bulk/buffering_adder.go, line 58 at r1 (raw file):
opts storagebase.BulkAdderOptions, ) (*BufferingAdder, error) { if opts.BufferSize == 0 {
Should we keep the check for negative flushBytes and sstBytes to prevent unintended usage?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsqlrun/bulk_row_writer.go, line 76 at r1 (raw file):
Previously, adityamaru27 (Aditya Maru) wrote…
was increasing the flushSize to 32MB intentional (since you're not passing it in as an option)?
whoops, nope... but I think I'm okay with it, since AFAIK, this wasn't picked for a specific reason so letting the adder use its default feels fine to me.
pkg/storage/bulk/buffering_adder.go, line 58 at r1 (raw file):
Previously, adityamaru27 (Aditya Maru) wrote…
Should we keep the check for negative flushBytes and sstBytes to prevent unintended usage?
could make them uint64 instead?
There was a problem hiding this comment.
Reviewable status:
pkg/storage/bulk/buffering_adder.go, line 58 at r1 (raw file):
Previously, dt (David Taylor) wrote…
could make them uint64 instead?
SGTM
adityamaru27
force-pushed
the
adder-config
branch
from
8f4e0a1
to
36969d6
Compare
The signature on this is hard to modify as it is plumbed though in several places, and as a result, many flags that control one behavior or another, like if SSTs should be added disallowing duplicates, were being mutated via 'Set' functions that were starting to get messy. By making a options struct that is in the signature used and plumbed through all the sql/distsql/etc layers, any new flags can be added here with minimal extra work involved. Release note: none.
adityamaru27
force-pushed
the
adder-config
branch
from
36969d6
to
c7b8288
Compare
|
bors r+ |
39496: storage/bulk,storagebase: add BulkAdderOptions struct r=adityamaru27 a=dt The signature on this is hard to modify as it is plumbed though in several places, and as a result, many flags that control one behavior or another, like if SSTs should be added disallowing duplicates, were being mutated via 'Set' functions that were starting to get messy. By making a options struct that is in the signature used and plumbed through all the sql/distsql/etc layers, any new flags can be added here with minimal extra work involved. Release note: none. Co-authored-by: David Taylor <tinystatemachine@gmail.com>
Build succeeded |
The signature on this is hard to modify as it is plumbed though in several places,
and as a result, many flags that control one behavior or another, like if SSTs
should be added disallowing duplicates, were being mutated via 'Set' functions
that were starting to get messy.
By making a options struct that is in the signature used and plumbed through all
the sql/distsql/etc layers, any new flags can be added here with minimal extra
work involved.
Release note: none.