server: storeCachedSettingsKVs removes unset settings value#101472
server: storeCachedSettingsKVs removes unset settings value#101472sladyn98 wants to merge 3 commits intocockroachdb:masterfrom
Conversation
This change removes the keys that are unset in storeCachedSettingsKVs. Earlier we did not clear out the values Fixes: cockroachdb#70567 <pkg>: <short description - lowercase, no final period> <what was there before: Previously, ...> <why it needed to change: This was inadequate because ...> <what you did about it: To address this, this patch ...>
|
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? Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. Before a member of our team reviews your PR, I have some potential action items for you:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
Thank you for updating your pull request. Before a member of our team reviews your PR, I have some potential action items for you:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Fixed CI errors
|
Thank you for updating your pull request. Before a member of our team reviews your PR, I have some potential action items for you:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
stevendanna
left a comment
There was a problem hiding this comment.
Thanks for looking into this! My sincere apologies for the delay.
I've left an initial question, but I need to do some digging around on the current state of the world to see if this is the direction that makes the most sense.
| existingKVs, err := storage.MVCCScan(ctx, eng, keys.LocalMax, roachpb.KeyMax, hlc.MaxTimestamp, storage.MVCCScanOptions{}) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
I believe the bounds of this scan should be keys.LocalStoreCachedSettingsKeyMin to keys.LocalStoreCachedSettingsKeyMax unless I'm misunderstanding?
|
We're choosing to solve this in a different and simpler way: #111475 Still, thank you for your contribution! |
111140: roachtest: harmonize GCE and AWS machine types r=erikgrinaker,herkolategan,renatolabs a=srosenberg Previously, same (performance) roachtest executed in GCE and AWS may have used a different memory (per CPU) multiplier and/or cpu family, e.g., cascade lake vs ice lake. In the best case, this resulted in different performance baselines on an otherwise equivalent machine type. In the worst case, this resulted in OOMs due to VMs in AWS having 2x less memory per CPU. This change harmozines GCE and AWS machine types by making them as isomorphic as possible, wrt memory, cpu family and price. The following heuristics are used depending on specified `MemPerCPU`: `Standard` yields 4GB/cpu, `High` yields 8GB/cpu, `Auto` yields 4GB/cpu up to and including 16 vCPUs, then 2GB/cpu. `Low` is supported _only_ in GCE. Consequently, `n2-standard` maps to `m6i`, `n2-highmem` maps to `r6i`, `n2-custom` maps to `c6i`, modulo local SSDs in which case `m6id` is used, etc. Note, we also force `--gce-min-cpu-platform` to `Ice Lake`; isomorphic AWS machine types are exclusively on `Ice Lake`. Roachprod is extended to show cpu family and architecture on `List`. Cost estimation now correctly deals with _custom_ machine types. Finally, we change the default zone allocation in GCE from exclusively `us-east1-b` to ~25% `us-central1-b` and ~75% `us-east1-b`. This is inteded to balance the quotas for local SSDs until we eventually switch to PD-SSDs. Epic: none Fixes: #106570 Release note: None 111442: server,kvcoord: change x-locality log from vevent to vinfo r=arulajmani a=kvoli The X-locality log events were added in #104585 to the Node batch receive path, to alert when localities were misconfigured. In some clusters, especially test clusters, these events are unnecessarily verbose in traces. Change the log from `VEvent(5)` to `VInfo(5)` in the node batch path. The X-locality log events were added in #103963 for the dist sender, to alert when localities were misconfigured. In some clusters, especially test clusters, these events are unnecessarily verbose in traces. Change the log from `VEvent(5)` to `VInfo(5)` in the dist sender path. Resolves: #110648 Epic: none Release note: None 111475: server,settingswatcher: fix the local persisted cache r=stevendanna,aliher1911 a=knz There's two commits here, fixing 2 separate issues. Epic: CRDB-6671 ### server,settingswatcher: properly evict entries from the local persisted cache Fixes #70567. Supersedes #101472. (For context, on each node there is a local persisted cache of cluster setting customizations. This exists to ensure that configured values can be used even before a node has fully started up and can start reading customizations from `system.settings`.) Prior to this patch, entries were never evicted from the local persisted cache: when a cluster setting was reset, any previously saved entry in the cache would remain there. This is a very old bug, which was long hidden and was recently revealed when commit 2f5d717 was merged. In a nutshell, before this recent commit the code responsible to load the entries from the cache didn't fully work and so the stale entries were never restored from the cache. That commit fixed the loader code, and so the stale entries became active, which made the old bug visible. To fix the old bug, this present commit modifies the settings watcher to preserve KV deletion events, and propagates them to the persisted cache. (There is no release note because there is no user-facing release where the bug was visible.) ### settingswatcher: write-through to the persisted cache Fixes #111422. Fixes #111328. Prior to this patch, the rangefeed watcher over `system.settings` was updating the in-RAM value store before it propagated the updates to the persisted local cache. In fact, the update to the persisted local cache was lagging quite a bit behind, because the rangefeed watcher would buffer updates and only flush them after a while. As a result, the following sequence was possible: 1. client updates a cluster setting. 2. server is immediately shut down. The persisted cache has not been updated yet. 3. server is restarted. For a short while (until the settings watcher has caught up), the old version of the setting remains active. This recall of ghost values of a setting was simply a bug. This patch fixes that, by ensuring that the persisted cache is written through before the in-RAM value store. By doing this, we give up on batching updates to the persisted local store. This is deemed acceptable because cluster settings are not updated frequently. Co-authored-by: Stan Rosenberg <stan.rosenberg@gmail.com> Co-authored-by: Austen McClernon <austen@cockroachlabs.com> Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
This change removes the keys that are unset in storeCachedSettingsKVs. Earlier we did not clear out the values
Fixes: #70567