changefeedccl: Respect changes to memory limits #96953
Assignees
Labels
A-cdc
Change Data Capture
C-enhancement
Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
O-support
Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs
T-cdc
Comments
miretskiy
added
C-enhancement
|
cc @cockroachdb/cdc |
jayshrivastava
added a commit
to jayshrivastava/cockroach
that referenced
this issue
Aug 21, 2023
Previously, changing `changefeed.memory.per_changefeed_limit` would require restarting a changefeed for the setting to take effect. This change makes it so that when the changefeed coordinator detects a change in memory limits, it restarts all the aggregators using a retryable error. Release note: None Informs: cockroachdb#96953 Epic: None
jayshrivastava
added a commit
to jayshrivastava/cockroach
that referenced
this issue
Aug 23, 2023
Previously, the kvfeed was responsible for monitoring for node drains using a goroutine. This change moves this logic into the change aggregator and removes the goroutine. Overall, this change makes the code more organized and performant. This change was inspired by work being done for cockroachdb#109167. The work in that PR requires being able to restart the kvfeed. Having drain logic intermingled with the kvfeed makes restarts much more complex, hard to review, prone to bugs, etc. Informs: cockroachdb#96953 Release note: None Epic: None
jayshrivastava
added a commit
to jayshrivastava/cockroach
that referenced
this issue
Aug 23, 2023
Previously, the kvfeed was responsible for monitoring for node drains using a goroutine. This change moves this logic into the change aggregator and removes the goroutine. Overall, this change makes the code more organized and performant. This change was inspired by work being done for cockroachdb#109167. The work in that PR requires being able to restart the kvfeed. Having drain logic intermingled with the kvfeed makes restarts much more complex, hard to review, prone to bugs, etc. Informs: cockroachdb#96953 Release note: None Epic: None
jayshrivastava
added a commit
to jayshrivastava/cockroach
that referenced
this issue
Aug 23, 2023
Previously, the kvfeed was responsible for monitoring for node drains using a goroutine. This change moves this logic into the change aggregator and removes the goroutine. Overall, this change makes the code more organized and performant. This change was inspired by work being done for cockroachdb#109167. The work in that PR requires being able to restart the kvfeed. Having drain logic intermingled with the kvfeed makes restarts much more complex, hard to review, prone to bugs, etc. Informs: cockroachdb#96953 Release note: None Epic: None
jayshrivastava
added a commit
to jayshrivastava/cockroach
that referenced
this issue
Aug 23, 2023
Previously, the kvfeed was responsible for monitoring for node drains using a goroutine. This change moves this logic into the change aggregator and removes the goroutine. Overall, this change makes the code more organized and performant. This change was inspired by work being done for cockroachdb#109167. The work in that PR requires being able to restart the kvfeed. Having drain logic intermingled with the kvfeed makes restarts much more complex, hard to review, prone to bugs, etc. Informs: cockroachdb#96953 Release note: None Epic: None
craig bot
pushed a commit
that referenced
this issue
Aug 23, 2023
…109356 108485: github: code coverage workflows r=RaduBerinde a=RaduBerinde This change adds two GitHub Action workflows which run on each PR. One generates unit test code coverage data, and one publishes that data to a GCS bucket from where Reviewable can access it. We generate coverage data using `bazel coverage`, but we restrict it to only test the packages that have been modified by the PR. Two workflows are required for security (the first workflow runs potentially malicious code from a fork); for more details, see https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ Epic: none Release note: None 109036: roachprod: better determination if scp -R flag can be used r=RaduBerinde a=RaduBerinde When uploading a file to a cluster, we use the "tree dist" algorithm by default. This uploads the file to a single node, then we copy the file from that node to the other nodes (up to 10). This only makes sense if the remote-to-remote transfers can happen directly, which only happens if we pass the `-R -A` flags to `scp`. Unfortunately older versions don't support these flags. Currently the flags are only passed if the OS is `darwin`. This commits improves the determination - we run `ssh -V` (once) and check if the `SSL` major version is three. For reference, some examples of what `ssh -V` returns: - recent MacOSX: `OpenSSH_9.0p1, LibreSSL 3.3.6` - Ubuntu 22.04: `OpenSSH_8.9p1 Ubuntu-3ubuntu0.3, OpenSSL 3.0.2 15 Mar 2022` In addition, if the version is not 3, we disable the use of "tree dist". Epic: none Release note: None 109260: sql stats: skip tests hitting combinedstmts and statements endpoints r=gtr a=gtr Part of #109184. This commit skips tests which hit the `combinedStmts` or `statements` endpoints which will sometimes timeout under stress as a result of recent backend changes. The test investigation is tracked by #109184. Release note: None 109288: dev: error when trying to `dev test` a bazel tested target r=rickystewart a=liamgillies Running `dev test` on these integration tests will always fail, so this PR adds a error when running the command on those files. Fixes: #107813 Release note: None 109292: sql: fix expected batch count for edge case in copy test r=rharding6373 a=rharding6373 In TestLargeDynamicRows we test that 4 rows of data can fit in a batch size of at least 4 rows given default memory sizes. However, when we set the batch row size to the minimum value of 4, the test hook that counts batches counts an extra empty batch. This PR changes adjusts the minimum row size to 5 for the purposes of this test. Epic: None Fixes: #109134 Release note: None 109324: build: update bazel builder build docs r=rickystewart a=rail Previously, the documentation described a manual build of the `bazelbuilder` docker image. The current approach is to use CI to build the image. This PR updates the documentation to reflect the current process, including the FIPS image steps. Epic: none Release note: None 109340: changefeedccl: move node drain handling logic out of kvfeed r=miretskiy a=jayshrivastava Previously, the kvfeed was responsible for monitoring for node drains using a goroutine. This change moves this logic into the change aggregator and removes the goroutine. Overall, this change makes the code more organized and performant. This change was inspired by work being done for #109167. The work in that PR requires being able to restart the kvfeed. Having drain logic intermingled with the kvfeed makes restarts much more complex, hard to review, prone to bugs, etc. Informs: #96953 Release note: None Epic: None 109349: kv: wait on latches on each key in reverse acquisition order r=arulajmani,kvoli a=nvanbenschoten This commit allocates latch IDs from the top of the uint64 space and in reverse order. This is done to order latches in the tree on a same key in reverse order of acquisition. Doing so ensures that when we iterate over the tree and see a key with many conflicting latches, we visit the latches on that key in the reverse order that they will be released. In doing so, we minimize the number of open channels that we wait on (calls to `waitForSignal`) and minimize the number of goroutine scheduling points. This is important to avoid spikes in runnable goroutine after each request completes, which can negatively affect node health. See experiments below. Epic: None Release note (performance improvement): The impact of high concurrency blind writes to the same key on goroutine scheduling latency was reduced. 109356: build: explicitly set SKIP_LABEL_TEST_FAILURE in compose.sh r=rickystewart a=chrisseto Previously, `SKIP_LABEL_TEST_FAILURE` was being set via a teamcity configuration. This change was quite opaque as the majority of CI configuration for Cockroach is stored as shell scripts within its repo. This commit follows that pattern by explicitly setting `SKIP_LABEL_TEST_FAILURE` in the script that runs `TestComposeCompare`. Epic: None Release note: None Co-authored-by: Radu Berinde <[email protected]> Co-authored-by: gtr <[email protected]> Co-authored-by: Liam Gillies <[email protected]> Co-authored-by: rharding6373 <[email protected]> Co-authored-by: Rail Aliiev <[email protected]> Co-authored-by: Jayant Shrivastava <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]> Co-authored-by: Chris Seto <[email protected]>
jayshrivastava
added a commit
to jayshrivastava/cockroach
that referenced
this issue
Aug 25, 2023
This change cleans up the code used to start up the kv feed in change aggregator processors. This change removes uncessessary code, adds a better API, and makes code easier to reason about. Informs: cockroachdb#96953 Release note: None Epic: None
craig bot
pushed a commit
that referenced
this issue
Aug 25, 2023
108597: metrics: assign histogram metric type on histogram construction r=ericharmeling a=ericharmeling This commit assigns prometheusgo.MetricType_HISTOGRAM to the Metadata.MetricType on histogram construction. Before this change, GetMetadata() was returning the Metadata.MetricType zero value (prometheusgo.MetricType_COUNTER) for all histograms that did not explicitly specify the prometheusgo.MetricType_HISTOGRAM for Metadata.MetricType in their Metadata definitions. This prevented checks on histogram Metadata.MetricType from properly evaluating the metrics as histograms. Fixes #106448. Fixes #107701. Releaes note: None 109345: changefeedccl: refactor kvfeed startup in changeaggregator processor r=miretskiy a=jayshrivastava changefeedccl: refactor kvfeed startup in changeaggregator processor This change cleans up the code used to start up the kv feed in change aggregator processors. This change removes uncessessary code, adds a better API, and makes code easier to reason about. Informs: #96953 Release note: None Epic: None 109386: sql: adjust many tests to work with test tenant r=yuzefovich a=yuzefovich Epic: CRDB-18499 Informs #76378 Release note: None 109476: dev: make `dev test --count 1` invalidate cached test results r=dt a=rickystewart This matches the behavior of `go test`. Epic: none Release note: None 109506: changefeedccl: ensure rangefeed setting is enabled in tests r=miretskiy a=jayshrivastava Previously, many tests which create rangefeeds would not explicitly set the `kv.rangefeed.enabled` setting to be true. These tests would still work because, by default, rangefeeds are enabled via span configs. However, it was observed that span configs are not immediately applied when range splits occur. This would cause the testing rangefeed reader to encounter errors and/or timeout on very rare occasions. See #109306 (comment) for more info. This change updates these tests to set the `kv.rangefeed.enabled` cluster setting to be true, which removes the dependency on span configs. Closes: #109306 Epic: None Release note: None 109511: concurrency: use generic lists in the lock table r=nvanbenschoten a=arulajmani Now that cda4fa2 has landed, we can make use of generic lists in a few places in the lock table. Epic: none Release note: None Co-authored-by: Eric Harmeling <[email protected]> Co-authored-by: Jayant Shrivastava <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Ricky Stewart <[email protected]> Co-authored-by: Arul Ajmani <[email protected]>
|
Unlikely we'll do this; seems a bit of a low priority. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Changes to
changefeed.memory.per_changefeed_limitsetting should be reflectedwithout the need to restart the changefeed.
Currently, the changes to this setting are picked up only after restarting the changefeed
because the parent memory monitor is initialized in the aggregator once.
Jira issue: CRDB-24430
Epic CRDB-24727
The text was updated successfully, but these errors were encountered: