closedts: make settings TenantReadOnly and public#108678
closedts: make settings TenantReadOnly and public#108678craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
|
We can't easily do this, because stuff like |
b88740e to
9b297f6
Compare
9b297f6 to
bb093ce
Compare
bb093ce to
3fd8927
Compare
|
Downgrading to draft while I work through the test failures. |
71a8886 to
aa30d28
Compare
7a7129c to
19609d2
Compare
|
@aliher1911 This should be good to go. It's a prereq for #108667. cc @nvanbenschoten in case you have opinions on closed timestamp settings. |
| // TargetDuration is the follower reads closed timestamp update target duration. | ||
| var TargetDuration = settings.RegisterDurationSetting( | ||
| settings.TenantWritable, | ||
| settings.TenantReadOnly, |
There was a problem hiding this comment.
Do we need to call this out as a breaking change? If users are erroneously setting these in their tenants in automation, they will now get an error.
There was a problem hiding this comment.
It only affects serverless users. Not sure how we communicate breaking changes there, but release notes don't seem like the best channel. Will find out.
There was a problem hiding this comment.
Added a release note.
nvb
left a comment
There was a problem hiding this comment.
cc @nvanbenschoten in case you have opinions on closed timestamp settings.
The change here 
Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @aliher1911 and @stevendanna)
19609d2 to
2d7af84
Compare
|
TFTR! bors r+ |
It doesn't make sense for these to be `TenantWritable`, since the side transport runs below KV. Furthermore, these settings are referenced throughout our documentation, so make them public. These should really be set only for the system tenant, and secondary tenants could simply read the system tenant's setting. This functionality runs in the host cluster below KV and it doesn't make any sense to set individual settings for tenants here. Unfortunately, this isn't currently possible with the existing settings classes, there is no way for secondary tenants to access the host's settings. Epic: none Release note (ops change): The following closed timestamp side-transport settings can no longer be set from secondary tenants (they did not have an effect in secondary tenants): kv.closed_timestamp.target_duration, kv.closed_timestamp.side_transport_interval, and kv.closed_timestamp.lead_for_global_reads_override.
2d7af84 to
3c6f442
Compare
|
Canceled. |
|
bors r+ |
|
Build succeeded: |
It doesn't make sense for these to be
TenantWritable, since the side transport runs below KV. Furthermore, these settings are referenced throughout our documentation, so make them public.These should really be set only for the system tenant, and secondary tenants could simply read the system tenant's setting. This functionality runs in the host cluster below KV and it doesn't make any sense to set individual settings for tenants here. Unfortunately, this isn't currently possible with the existing settings classes, there is no way for secondary tenants to access the host's settings.
Touches #108677.
Epic: none
Release note (ops change): The following closed timestamp side-transport settings can no longer be set from secondary tenants (they did not have an effect in secondary tenants): kv.closed_timestamp.target_duration, kv.closed_timestamp.side_transport_interval, and kv.closed_timestamp.lead_for_global_reads_override.