closedts: shorten target_duration from 30s to 3s#43147
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Dec 17, 2019
Merged
closedts: shorten target_duration from 30s to 3s#43147craig[bot] merged 1 commit intocockroachdb:masterfrom
craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
Member
|
This change is |
1ab8bb1 to
a667e08
Compare
Contributor
Author
|
@nvanbenschoten thoughts on getting this merged sooner rather than later? With these settings we'll send a closed timestamp update every 600ms. That's starting to get pretty high from a chattiness perspective. I'm hopeful that later in this release we'll get more sophisticated with regards to how closed timestamp information is propagated to lower the trailing interval dramatically further and ideally make the protocol less chatty. In the meantime I'd like to shake out any implications of having a shorter window. |
nvb
approved these changes
Dec 14, 2019
Contributor
nvb
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained
Contributor
Author
|
🤞 bors r=nvanbenschoten |
Contributor
Build failed |
Contributor
Author
|
flaked on bors r+ |
Contributor
Build failed |
This PR dramatically shortens the closed timestamp target interval. With this setting the experimental_follower_read_timestamp() will now be 4.8 seconds in the past as opposed to the current 48 seconds in the past. This relatively aggressive setting of 3s is intended to shake out issues. This value has been verified to work on the `schemachange/mixed/tpcc` roachtest introduced in cockroachdb#39096 and for vanilla TPC-C runs around where we have established a baseline in the past using tpccbench. Specifically several runs at 2300 warehouses on 3x c5d.4xlarge nodes which is right at the passing boundary without the change were run only a very small difference in efficiency or tail latency was observed. It seems reasonable to attempt to live with this value on master for a while and see what happens. Fixes cockroachdb#37083. Release note (enterprise change): Shorten the default closed timestamp target interval from 30s to 3s leading to permit follower reads at 4.8 seconds in the past rather than the previous 48 seconds.
a667e08 to
797819b
Compare
Contributor
|
I'm 100% in favor of this, but please do check in on the nightly roachtest failures for the day or two after it merges. |
Contributor
Author
Will do. bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Dec 17, 2019
43147: closedts: shorten target_duration from 30s to 3s r=ajwerner a=ajwerner This PR dramatically shortens the closed timestamp target interval. With this setting the experimental_follower_read_timestamp() will now be 4.8 seconds in the past as opposed to the current 48 seconds in the past. This relatively aggressive setting of 3s is intended to shake out issues. This value has been verified to work on the `schemachange/mixed/tpcc` roachtest introduced in #39096 and for vanilla TPC-C runs around where we have established a baseline in the past using tpccbench. Specifically several runs at 2300 warehouses on 3x c5d.4xlarge nodes which is right at the passing boundary without the change were run only a very small difference in efficiency or tail latency was observed. It seems reasonable to attempt to live with this value on master for a while and see what happens. Fixes #37083. Release note (enterprise change): Shorten the default closed timestamp target interval from 30s to 3s leading to permit follower reads at 4.8 seconds in the past rather than the previous 48 seconds. 43226: sql: fix namespace name resolution fallback code r=arulajmani a=arulajmani Previously, the fallback for name resolution was broken. If an entry is not present in the new `system.namespace`, we should look it up in the old `system.namespace`. This was not happening, as we would only fallback to the old `system.namespace` if the entry was a table. Thus, databases created before the new `system.namespace` table was created were not being resolved at all. This PR fixes that. Fixes #43141 Release note: None Co-authored-by: Andrew Werner <ajwerner@cockroachlabs.com> Co-authored-by: arulajmani <arulajmani@gmail.com>
Contributor
Build succeeded |
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 PR dramatically shortens the closed timestamp target interval. With this
setting the experimental_follower_read_timestamp() will now be 4.8 seconds in
the past as opposed to the current 48 seconds in the past. This relatively
aggressive setting of 3s is intended to shake out issues.
This value has been verified to work on the
schemachange/mixed/tpccroachtestintroduced in #39096 and for vanilla TPC-C runs around where we have
established a baseline in the past using tpccbench. Specifically several runs
at 2300 warehouses on 3x c5d.4xlarge nodes which is right at the passing
boundary without the change were run only a very small difference in
efficiency or tail latency was observed.
It seems reasonable to attempt to live with this value on master for a while
and see what happens.
Fixes #37083.
Release note (enterprise change): Shorten the default closed timestamp target
interval from 30s to 3s leading to permit follower reads at 4.8 seconds in
the past rather than the previous 48 seconds.