-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
changefeedccl: changefeed concurrency and frontier observability #67206
Conversation
3561563
to
ef69840
Compare
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.
Reviewed 2 of 3 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @miretskiy)
pkg/ccl/changefeedccl/changefeedbase/settings.go, line 88 at r1 (raw file):
// If set to 0, a reasonable default will be chosen. var ScanRequestLimit = settings.RegisterIntSetting( "changfeed.backfill.concurrent_scan_requests",
s/changfeed/changefeed
pkg/ccl/changefeedccl/kvfeed/scanner.go, line 271 at r1 (raw file):
if err != nil { // can't count nodes in tenants nodes = 1
Don't you need to immediately return 3 here? Otherwise you're using the value of g
below which can be nil.
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.
Reviewed 1 of 3 files at r1, 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @miretskiy)
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 @ajwerner and @HonoreDB)
pkg/ccl/changefeedccl/changefeedbase/settings.go, line 88 at r1 (raw file):
Previously, HonoreDB (Aaron Zinger) wrote…
s/changfeed/changefeed
Done.
pkg/ccl/changefeedccl/kvfeed/scanner.go, line 271 at r1 (raw file):
Previously, HonoreDB (Aaron Zinger) wrote…
Don't you need to immediately return 3 here? Otherwise you're using the value of
g
below which can be nil.
good one. Fixed.
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 @ajwerner, @HonoreDB, and @miretskiy)
pkg/ccl/changefeedccl/changefeedbase/settings.go, line 89 at r3 (raw file):
var ScanRequestLimit = settings.RegisterIntSetting( "changefeed.backfill.concurrent_scan_requests", "number of concurrent scan requests during a backfill",
Should we specify that this is the number of requests per node?
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 @ajwerner, @HonoreDB, and @stevendanna)
pkg/ccl/changefeedccl/changefeedbase/settings.go, line 89 at r3 (raw file):
Previously, stevendanna (Steven Danna) wrote…
Should we specify that this is the number of requests per node?
done
Stop relying on ExportRequestLimit to determine the number of concurrent export requests, and introduce a decidated ScanRequestLimit setting. If the setting is specified, uses that setting; otherwise, the default value is computed as 3 * (number of nodes in the cluster), which is the old behavior, but we cap this number so that concurrency does not get out of hand if running in a very large cluster. Fixes cockroachdb#67190 Release Nodes: Provide a better configurability of scan request concurrency. Scan requests are issued by changefeeds during the backfill.
Add a metric to keep track of the number of frontier updates in the changefeed. Add logging when job progress updates take excessive amount of time. Fixes cockroachdb#67192 Release Notes: None
67268: changefeedccl: changefeed concurrency and frontier observability #67206 r=miretskiy a=miretskiy Stop relying on ExportRequestLimit to determine the number of concurrent export requests, and introduce a decidated ScanRequestLimit setting. If the setting is specified, uses that setting; otherwise, the default value is computed as 3 * (number of nodes in the cluster), which is the old behavior, but we cap this number so that concurrency does not get out of hand if running in a very large cluster. Fixes #67190 Improve Observability of of change frontier updates. Add a metric to keep track of the number of frontier updates in the changefeed. Add logging when job progress updates take excessive amount of time. Fixes #67192 Release Nodes: Provide a better configurability of scan request concurrency. Scan requests are issued by changefeeds during the backfill. Co-authored-by: Yevgeniy Miretskiy <[email protected]>
Stop relying on ExportRequestLimit to determine the number of concurrent
export requests, and introduce a decidated ScanRequestLimit setting.
If the setting is specified, uses that setting; otherwise, the default
value is computed as 3 * (number of nodes in the cluster), which is the
old behavior, but we cap this number so that concurrency does not get
out of hand if running in a very large cluster.
Fixes #67190
Improve Observability of of change frontier updates.
Add a metric to keep track of the number of frontier updates in the
changefeed. Add logging when job progress updates take excessive amount
of time.
Fixes #67192
Release Nodes: Provide a better configurability of scan request
concurrency. Scan requests are issued by changefeeds during the
backfill.