Skip to content
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

kv: remove kv.snapshot_recovery.max_rate setting #102596

Merged

Conversation

andrewbaptist
Copy link
Contributor

@andrewbaptist andrewbaptist commented Apr 28, 2023

Fixes: #63728

There were two settings that controlled snapshot transfer rates, however, this has caused numerous problems when they are set differently as the code makes timeout assumptions based on this rate.

Epic: none

Release note: This change removes a user configurable setting
kv.snapshot_recovery.max_rate. Guidance to customers has been to
always set this equal to kv.snapshot_rebalance.max_rate and this
change will start using that setting for all snapshots. If the customer
previously set kv.snapshot_recovery.max_rate it will be cleared after
this is released and future attempts to set it will fail with:
"ERROR: unknown cluster setting 'kv.snapshot_recovery.max_rate'

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@andrewbaptist andrewbaptist force-pushed the 2023-04-28-remove-recovery-rate branch from 898e754 to 1950b2e Compare April 28, 2023 15:19
@andrewbaptist andrewbaptist marked this pull request as ready for review April 28, 2023 16:03
@andrewbaptist andrewbaptist requested a review from a team April 28, 2023 16:03
@andrewbaptist andrewbaptist requested a review from a team as a code owner April 28, 2023 16:03
@andrewbaptist andrewbaptist requested a review from tbg April 28, 2023 16:03
@andrewbaptist andrewbaptist force-pushed the 2023-04-28-remove-recovery-rate branch from 1950b2e to ebbcd1f Compare April 28, 2023 16:34
@andrewbaptist andrewbaptist requested a review from a team as a code owner April 28, 2023 16:34
@andrewbaptist andrewbaptist requested review from herkolategan and renatolabs and removed request for a team April 28, 2023 16:34
@andrewbaptist andrewbaptist force-pushed the 2023-04-28-remove-recovery-rate branch from ebbcd1f to f39f9db Compare April 28, 2023 18:12
Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙇🏽

}
return nil
},
).WithPublic()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we "Retire" that metric before just ripping it out?

// SetRetired marks the setting as obsolete. It also hides
// it from the output of SHOW CLUSTER SETTINGS.
func (c *common) SetRetired() {
c.description = "do not use - " + c.description
c.retired = true
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an end to end example

cockroach/pkg/ts/db.go

Lines 63 to 72 in fb09177

// deprecatedResolution30StoreDuration is retained for backward compatibility during a version upgrade.
var deprecatedResolution30StoreDuration = func() *settings.DurationSetting {
s := settings.RegisterDurationSetting(
settings.TenantWritable,
"timeseries.storage.30m_resolution_ttl", "replaced by timeseries.storage.resolution_30m.ttl",
resolution30mDefaultPruneThreshold,
)
s.SetRetired()
return s
}()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I changed to retired. and cleaned it up. It ended up being much better to use in the init function as it doesn't assign it a variable name (it is provably retired, not just a naming convention).

@andrewbaptist andrewbaptist force-pushed the 2023-04-28-remove-recovery-rate branch from f39f9db to 81c31a8 Compare April 28, 2023 20:56
Fixes: cockroachdb#63728

There were two settings that controlled snapshot transfer rates, however
this has caused numerous problems when they are set differently as
the code makes timeout assumptions based on this rate.

Epic: none

Release note:
This change removes a user configurable setting
`kv.snapshot_recovery.max_rate`. Guidance to customers has been to
always set this equal to `kv.snapshot_rebalance.max_rate` and this
change will start using that setting for all snapshots. If the customer
previously set `kv.snapshot_recovery.max_rate` it will be cleared after
this is released and future attempts to set it will fail with:
"ERROR: unknown cluster setting 'kv.snapshot_recovery.max_rate'
@andrewbaptist andrewbaptist force-pushed the 2023-04-28-remove-recovery-rate branch from 81c31a8 to 2980cd6 Compare April 28, 2023 21:06
@tbg tbg self-requested a review May 1, 2023 06:17
@andrewbaptist
Copy link
Contributor Author

bors r=tbg

@craig
Copy link
Contributor

craig bot commented May 1, 2023

Build succeeded:

@craig craig bot merged commit fd33b0a into cockroachdb:master May 1, 2023
@shralex shralex added the O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs label May 14, 2023
@andrewbaptist andrewbaptist deleted the 2023-04-28-remove-recovery-rate branch December 15, 2023 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kv: Clean up snapshot rate limits
4 participants