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.

Sign up for GitHub

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

Improve cache_size and cache_size_percent descriptions #696

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+3 −3
Open

Improve cache_size and cache_size_percent descriptions #696

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c22368f
Improve cache_size and cache_size_percent descriptions
daisukebe Aug 21, 2024
ea2e992
Update modules/reference/pages/properties/object-storage-properties.adoc
daisukebe Aug 22, 2024
02c37e9
Update modules/reference/pages/properties/object-storage-properties.adoc
daisukebe Aug 22, 2024
16f7518
Merge branch 'main' into cache-size-percent
kbatuigas Aug 22, 2024
25d3898
Update modules/reference/pages/properties/object-storage-properties.adoc
daisukebe Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions modules/reference/pages/properties/object-storage-properties.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ AWS or GCS bucket that should be used to store data.

Max size of object storage cache.

If both this property and <<cloud_storage_cache_size_percent>> are set, Redpanda uses the minimum of the two.
If both this property and <<cloud_storage_cache_size_percent>> are set, Redpanda uses the minimum of the two. If one of them is set to 0, Redpanda uses the non-zero one. If they are both set to 0, the cache size is 0.
Copy link

Choose a reason for hiding this comment

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

Before making this change, I'd want to see what happens if the cache size is actually 0. This makes it sound like we support a cache size of 0, but I imagine cloud storage just wont work below some minimal cache size.

Copy link
Contributor

Choose a reason for hiding this comment

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

any resolution on this @jcipar @daisukebe ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given there's no guardrails on setting to zero from preventing, we should document like below?

If both this property and <<cloud_storage_cache_size_percent>> are set, Redpanda uses the minimum of the two. If one of them is set to 0, Redpanda uses the non-zero one. Redpanda strongly recommends you do not set both of them to zero.

@jcipar @WillemKauf ^

Copy link
Contributor

Choose a reason for hiding this comment

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

@jcipar and @WillemKauf Looks like this PR is stuck and awaiting your guidance here. Please respond asap. Thx.

Copy link

Choose a reason for hiding this comment

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

Hi @Feediver1, I just put out a PR that will update the behaviour here and (hopefully) make the update to the docs more straight-forward. We will now prevent the following invalid configurations:

  1. cloud_storage_cache_size and cloud_storage_cache_size_percent cannot both be 0
  2. cloud_storage_cache_size cannot be 0 while cloud_storage_cache_size_percent is nil (std::nullopt)

I would say something like:

If both this property and <<cloud_storage_cache_size_percent>> are set, Redpanda uses the minimum of the two. If one of them is set to 0, Redpanda uses the non-zero one. These properties cannot both be `0` simultaneously.

Hope this helps.

Copy link
Contributor

Choose a reason for hiding this comment

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

@WillemKauf @jcipar - The description has now been updated to reflect the change in the code. Please could you take a look and approve.

Also, please provide guidance on what happens when both properties are set to 0 so that we can provide troubleshooting advice where necessary.

Copy link

Choose a reason for hiding this comment

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

If the user attempts to set both configs to 0 via rpk cluster config edit, the configuration will be rejected (not applied) and the error message Cannot set both cloud_storage_cache_size and cloud_storage_cache_size_pct to 0. will be provided.

daisukebe marked this conversation as resolved.
Show resolved Hide resolved

*Units*: bytes

Expand Down Expand Up @@ -593,7 +593,7 @@ Divide the object storage cache across the specified number of buckets. This onl

Maximum size of the cloud cache as a percentage of unreserved disk space (see config_ref:disk_reservation_percent,true,cluster-properties[]). The default value for this option is tuned for a shared disk configuration. Consider increasing the value if using a dedicated cache disk.

The property <<cloud_storage_cache_size>> controls the same limit expressed as a fixed number of bytes. If both `cloud_storage_cache_size` and `cloud_storage_cache_size_percent` are set, Redpanda uses the minimum of the two.
The property <<cloud_storage_cache_size>> controls the same limit expressed as a fixed number of bytes. If both `cloud_storage_cache_size` and `cloud_storage_cache_size_percent` are set, Redpanda uses the minimum of the two. If one of them is set to 0, Redpanda uses the non-zero one. If they are both set to 0, the cache size is 0.
daisukebe marked this conversation as resolved.
Show resolved Hide resolved

*Units*: percentage of total disk size.

Expand Down Expand Up @@ -1706,4 +1706,4 @@ If neither addressing style works, Redpanda terminates the startup, requiring ma

*Default:* `null`

---
---