server: move settingswatcher initialization to the start #97991
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 |
|
I extracted this from #97406 so it can be reviewed on its own. It is generally useful to have the settings values earlier in the initialization flow, but it is really important for cluster version. Subsystems interacting with sql_instance, sqlliveness, and lease tables need an up to date version in order to know whether the new or the old index should be used. |
There will be meaningful test coverage for this once version gates are added for the sqlliveness, sql_instances, and lease tables. |
|
I'll look at this when i am back at the desk. In the meantime, I'll share a thought developed with @arulajmani : extend the setting watcher initialization with logic to wait until the initial batch of overrides is received, using a sentinel. This ensures we wait until the initial config is fully applied. I believed this will be useful for the version setting between other things. |
|
I looked into the TestTenantUnauthenticatedAccess test failure. There is a minor bug in the rangefeedcache package used by the settingwatcher. The rangfeedcache attempts to treat an auth error as a permanent error, but the error never gets reported to the goroutine setting up the watcher and it causes everything to hang. |
jeffswenson
force-pushed
the
jeffswenson-reorder-settings-initialization
branch
from
fd99338 to
4d2939e
Compare
Previously, if the rangfeedcache.Watcher ran into an auth error, it was never reported to the caller. This was detected as part of cockroachdb#97991. Moving the settingswatcher to be the first component initialized in the sql server caused the TestTenantUnauthenticatedAccess test to fail. Now, the error is reported to the caller and rebasing cockroachdb#97991 on top of this change allows the test to pass. Part of cockroachdb#94843 Release note: None
jeffswenson
force-pushed
the
jeffswenson-reorder-settings-initialization
branch
from
4d2939e to
31bfdaa
Compare
|
I opened #98111 to fix the bug in rangefeedcache and rebased this change on top of it. |
Previously, if the rangfeedcache.Watcher ran into an auth error, it was never reported to the caller. This was detected as part of cockroachdb#97991. Moving the settingswatcher to be the first component initialized in the sql server caused the TestTenantUnauthenticatedAccess test to fail. Now, the error is reported to the caller and rebasing cockroachdb#97991 on top of this change allows the test to pass. Part of cockroachdb#94843 Release note: None
97805: settings: add COCKROACH_IGNORE_CLUSTER_SETTINGS env var r=dt a=dt This provides an override usable in siturations where the persisted values for cluster settings may be causing a problem such that one cannot even start and interact with the the cluster enough to update them, such as if a given persisted setting causes nodes to crash on start or reject all SQL connections. In such cases, this env var provides a mechanism to start a node which will ignore the persisted values for all cluster settings and operate with just the default values instead, allowing alterations of the persisted values (though those alterations will similarly be ignored). Release note (ops change): The enviornment variable COCKROACH_IGNORE_CLUSTER_SETTINGS can be used to start a node that ignores all stored cluster setting values in emergencies. Epic: none. 97948: opt: don't fold sub-operators with null operands during type check r=DrewKimball a=DrewKimball This commit prevents type-checking from replacing expressions like `NULL = ANY(...)` with `NULL`. This is necessary because in the case when the right operand is an empty array, the result of the expression is `False` instead of `NULL`. It is not always possible to know what the right operand will evaluate to, and constant folding can be handled during normalization anyway. Fixes #37793 Release note (bug fix): Fixed a long-standing and rare bug in evaluation of `ANY`, `SOME`, and `ALL` sub-operators that would cause expressions like `NULL = ANY(ARRAY[]::INT[])` to return `NULL` instead of `False`. 98111: rangefeedcache: handle auth error in initial scan r=JeffSwenson a=JeffSwenson Previously, if the rangfeedcache.Watcher ran into an auth error, it was never reported to the caller. This was detected as part of #97991. Moving the settingswatcher to be the first component initialized in the sql server caused the TestTenantUnauthenticatedAccess test to fail. Now, the error is reported to the caller and rebasing #97991 on top of this change allows the test to pass. Part of #94843 Release note: None 98143: ui: remove timeScale update from fingerprint details link from insight details r=ericharmeling a=ericharmeling Part of #97952. This PR removes the setTimeScale call from the StatementDetailsLink and TransactionDetailsLink functions. Navigating to the Fingerprint Details pages from the Insights Details pages will no longer set the global time scale. Loom (CC Console): https://www.loom.com/share/ca388c8c0068446d960f89cc6c1f6af5 Loom (DB Console): https://www.loom.com/share/209f2d21a1784d1d86adffabddb9b47f Epic: none Release note: None Co-authored-by: David Taylor <[email protected]> Co-authored-by: Drew Kimball <[email protected]> Co-authored-by: Jeff <[email protected]> Co-authored-by: Eric Harmeling <[email protected]>
Previously, the settingswatcher was one of the last services initialized during sql server start up. Now, it is one of the first services to start up. The settings do not have a well defined value until after settingwatcher initializes. Some settings, like cluster version, must have valid values during initialization. Part of cockroachdb#94843
jeffswenson
force-pushed
the
jeffswenson-reorder-settings-initialization
branch
from
31bfdaa to
bea6fb6
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @jeffswenson)
|
bors r+ |
I decided to merge this for now as I have some changes rebased on top. I agree the ideal behavior is Start should block on fetching overrides from the host cluster. I reviewed the implementation and settings stored within the tenant appear to be handled correctly: all of the settings are read before Start returns. |
|
Build succeeded: |
Previously, the settingswatcher was one of the last services initialized during sql server start up. Now, it is one of the first services to start up. The settings do not have a well defined value until after settingwatcher initializes. Some settings, like cluster version, must have valid values during initialization.
Part of #94843