kvserver: default kv.rangefeed.closed_timestamp_refresh_interval to 3s#108667
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Aug 22, 2023
Merged
Conversation
Member
|
This change is |
Contributor
Author
|
I could also be convinced to set this to 2-3s. Wdyt @miretskiy? |
Contributor
Author
|
Getting a bunch of test failures here because |
43764bd to
689a596
Compare
kv.rangefeed.closed_timestamp_refresh_interval to 1skv.rangefeed.closed_timestamp_refresh_interval to 3s
Contributor
Author
|
Decided to bump this to 3 seconds instead of 1 second, considering the changefeed checkpoint default is 30 seconds. |
miretskiy
approved these changes
Aug 14, 2023
8e50065 to
82605fb
Compare
This patch sets `kv.rangefeed.closed_timestamp_refresh_interval` to 3s by default. Previously, it defaulted to 0, which fell back to `kv.closed_timestamp.side_transport_interval` at 200ms by default. The setting is also made public, since users may often have to tune this. It is also made TenantReadOnly, since tenants need to access the host's setting, and the only current option is to set an explicit setting for the tenant since they can't directly access host settings. Frequent rangefeed closed timestamp processing can have a significant cost with many ranges, and the 200ms interval was far lower than the default target lag (3s) and changefeed checkpoint interval (30s), so this work was for little benefit. `kv.closed_timestamp.side_transport_interval` has not been modified, since this would affect the latency of follower reads and global tables. Release note (ops change): The rangefeed closed timestamp interval controlled by `kv.rangefeed.closed_timestamp_refresh_interval` now defaults to 3 second. This affects how often rangefeeds emit resolved timestamps, and thus how often changefeeds can emit checkpoints. Previously, its default value of 0 would fall back to `kv.closed_timestamp.side_transport_interval`, which defaults to 200 milliseconds. Users who rely on the setting `kv.closed_timestamp.side_transport_interval` to control the rangefeed closed timestamp interval should make sure they either set `kv.rangefeed.closed_timestamp_refresh_interval` to 0 to retain the old behavior (preferably before upgrading), or to an appropriate value.
82605fb to
e1fc2dc
Compare
aliher1911
approved these changes
Aug 22, 2023
Contributor
Author
|
bors r+ |
Contributor
|
Build succeeded: |
arulajmani
added a commit
to arulajmani/cockroach
that referenced
this pull request
Sep 5, 2023
Previously, we weren't waiting for the PTS record to make its way to KV in this test. This meant expecting it to provide protection (which the test asserted) was racy with spanconfig reconciliation. Span config reconciliation is built over rangefeeds, so when we bumped the default value of `kv.rangefeed.closed_timestamp_refresh_interval` in cockroachdb#108667, it meant span config reconciliation would take longer. This caused the GC queue to run before the replica was aware of the PTS record, which violated a test assertion. This patch fixes the issue by only enqueueing the range in the GC queue once the PTS record is known to have made it to KV. Closes cockroachdb#109244 Release note: None
craig bot
pushed a commit
that referenced
this pull request
Sep 5, 2023
110072: kvserver: deflake TestProtectedTimestamps r=irfansharif a=arulajmani Previously, we weren't waiting for the PTS record to make its way to KV in this test. This meant expecting it to provide protection (which the test asserted) was racy with spanconfig reconciliation. Span config reconciliation is built over rangefeeds, so when we bumped the default value of `kv.rangefeed.closed_timestamp_refresh_interval` in #108667, it meant span config reconciliation would take longer. This caused the GC queue to run before the replica was aware of the PTS record, which violated a test assertion. This patch fixes the issue by only enqueueing the range in the GC queue once the PTS record is known to have made it to KV. Closes #109244 Release note: None Co-authored-by: Arul Ajmani <arulajmani@gmail.com>
This was referenced Sep 25, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patch sets
kv.rangefeed.closed_timestamp_refresh_intervalto 3s by default. Previously, it defaulted to 0, which fell back tokv.closed_timestamp.side_transport_intervalat 200ms by default.The setting is also made public, since users may often have to tune this. It is also made TenantReadOnly, since tenants need to access the host's setting, and the only current option is to set an explicit setting for the tenant since they can't directly access host settings.
Frequent rangefeed closed timestamp processing can have a significant cost with many ranges, and the 200ms interval was far lower than the default target lag (3s) and changefeed checkpoint interval (30s), so this work was for little benefit.
kv.closed_timestamp.side_transport_intervalhas not been modified, since this would affect the latency of follower reads and global tables.Resolves #108656.
Release note (ops change): The rangefeed closed timestamp interval controlled by
kv.rangefeed.closed_timestamp_refresh_intervalnow defaults to 3 second. This affects how often rangefeeds emit resolved timestamps, and thus how often changefeeds can emit checkpoints. Previously, its default value of 0 would fall back tokv.closed_timestamp.side_transport_interval, which defaults to 200 milliseconds. Users who rely on the settingkv.closed_timestamp.side_transport_intervalto control the rangefeed closed timestamp interval should make sure they either setkv.rangefeed.closed_timestamp_refresh_intervalto 0 to retain the old behavior (preferably before upgrading), or to an appropriate value.