bulk,importccl: re-read settings during ingestion#42268
bulk,importccl: re-read settings during ingestion#42268craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
ajwerner
left a comment
There was a problem hiding this comment.
Just the one comment, take it or leave it.
Reviewed 8 of 8 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @dt and @miretskiy)
pkg/storage/bulk/sst_batcher.go, line 307 at r1 (raw file):
b.flushedToCurrentRange = size } if b.splitAfter != nil && b.flushedToCurrentRange > b.splitAfter() && nextKey != nil {
This changes the meaning of a zero value split after. Do you want to keep the behavior of zero disabling the behavior ?
dt
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/bulk/sst_batcher.go, line 307 at r1 (raw file):
Previously, ajwerner wrote…
This changes the meaning of a zero value split after. Do you want to keep the behavior of zero disabling the behavior ?
My thinking was that the zero-value of a func() is nil, and that still has the same meaning.
I guess though if you hooked it up to a cluster setting (it isn’t currently) the user’s only choice would be to set to 0 and not eliminate the func altogether, so maybe I do want to check non-nil and >0.
Will do.
miretskiy
left a comment
There was a problem hiding this comment.
Reviewed 8 of 8 files at r1.
Reviewable status:
b1b0a86 to
2ba3829
Compare
Prior to this patch, the bulk-ingestion helpers were configured with size limits read from settings, but then executed, sometimes for hours, against those values that were passed in, not reacting to later changes in the underlying setting. This change plumbs those values though as func() int64 instead of int64, re-reading the setting value each time it is used and thus reflecting any changes to the underlying setting during execution. Also, while I'm here: switch uint64 to int64 to match our usual style guidelines. Release note (bug fix): fix bug where some settings changes were not reflected during currently running IMPORTs.
|
bors r+ |
Build failed (retrying...) |
42225: storage/engine: remove runWithAllEngines r=petermattis a=petermattis Replace usage of `runWithAllEngines` with the more widely used for-loop over `mvccEngineImpls`. Fixes #42060 Release note: None 42235: roachtest: update gopg ORM test r=yuzefovich a=yuzefovich There was a new release of gopg ORM which included a couple of PRs that improved the tool's testing infrastructure. Namely, the new release has several environment variables that can override the default options, and now we will use those. Also, there was a minor modification of error matching, so we can remove one of the tests from the ignore lists. Fixes: #41890. Release note: None 42268: bulk,importccl: re-read settings during ingestion r=dt a=dt Prior to this patch, the bulk-ingestion helpers were configured with size limits read from settings, but then executed, sometimes for hours, against those values that were passed in, not reacting to later changes in the underlying setting. This change plumbs those values though as func() int64 instead of int64, re-reading the setting value each time it is used and thus reflecting any changes to the underlying setting during execution. Also, while I'm here: switch uint64 to int64 to match our usual style guidelines. Release note (bug fix): fix bug where some settings changes were not reflected during currently running IMPORTs. Co-authored-by: Peter Mattis <petermattis@gmail.com> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: David Taylor <tinystatemachine@gmail.com>
Build succeeded |
Prior to this patch, the bulk-ingestion helpers were configured with size limits read from settings,
but then executed, sometimes for hours, against those values that were passed in, not reacting to
later changes in the underlying setting.
This change plumbs those values though as func() int64 instead of int64, re-reading the setting value
each time it is used and thus reflecting any changes to the underlying setting during execution.
Also, while I'm here: switch uint64 to int64 to match our usual style guidelines.
Release note (bug fix): fix bug where some settings changes were not reflected during currently running IMPORTs.