Skip to content

server: storeCachedSettingsKVs removes unset settings value#101472

Closed
sladyn98 wants to merge 3 commits intocockroachdb:masterfrom
sladyn98:remove-unset-values
Closed

server: storeCachedSettingsKVs removes unset settings value#101472
sladyn98 wants to merge 3 commits intocockroachdb:masterfrom
sladyn98:remove-unset-values

Conversation

@sladyn98
Copy link
Copy Markdown

This change removes the keys that are unset in storeCachedSettingsKVs. Earlier we did not clear out the values

Fixes: #70567

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 ...>
@sladyn98 sladyn98 requested a review from a team as a code owner April 13, 2023 17:23
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Apr 13, 2023

CLA assistant check
All committers have signed the CLA.

@blathers-crl
Copy link
Copy Markdown

blathers-crl bot commented Apr 13, 2023

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:

  • Please ensure your git commit message contains a release note.
  • When CI has completed, please ensure no errors have appeared.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@blathers-crl blathers-crl bot added the O-community Originated from the community label Apr 13, 2023
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@blathers-crl
Copy link
Copy Markdown

blathers-crl bot commented Apr 13, 2023

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

  • We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
  • Please ensure your git commit message contains a release note.
  • When CI has completed, please ensure no errors have appeared.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@blathers-crl
Copy link
Copy Markdown

blathers-crl bot commented Apr 13, 2023

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

  • We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
  • Please ensure your git commit message contains a release note.
  • When CI has completed, please ensure no errors have appeared.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Copy link
Copy Markdown
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +104 to +107
existingKVs, err := storage.MVCCScan(ctx, eng, keys.LocalMax, roachpb.KeyMax, hlc.MaxTimestamp, storage.MVCCScanOptions{})
if err != nil {
return err
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I believe the bounds of this scan should be keys.LocalStoreCachedSettingsKeyMin to keys.LocalStoreCachedSettingsKeyMax unless I'm misunderstanding?

@knz
Copy link
Copy Markdown
Contributor

knz commented Sep 29, 2023

We're choosing to solve this in a different and simpler way: #111475

Still, thank you for your contribution!

@knz knz closed this Sep 29, 2023
craig bot pushed a commit that referenced this pull request Sep 29, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

O-community Originated from the community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

server: storeCachedSettingsKVs does not remove unset settings values

4 participants