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

admin: add cloud_storage_cache_size check to config_multi_property_validation() #23337

Merged
merged 3 commits into from
Sep 18, 2024
Merged

admin: add cloud_storage_cache_size check to config_multi_property_validation() #23337

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ff73a96
`cloud_storage`: add `cloud_storage::cache::validate_cache_config()`
WillemKauf Sep 17, 2024
d4674d6
`admin`: add multi-property `cloud_storage_cache_size` check
WillemKauf Sep 16, 2024
0df8fdd
`rptest`: add `test_validate_cloud_storage_cache_size_config`
WillemKauf Sep 16, 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
23 changes: 23 additions & 0 deletions src/v/cloud_storage/cache_service.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1970,4 +1970,27 @@ ss::future<> cache::sync_access_time_tracker(
}
}

std::optional<ss::sstring>
cache::validate_cache_config(const config::configuration& conf) {
const auto& cloud_storage_cache_size = conf.cloud_storage_cache_size;
const auto& cloud_storage_cache_size_pct
= conf.cloud_storage_cache_size_percent;

// If not set, cloud cache uses default value of 0.0
auto cache_size_pct = cloud_storage_cache_size_pct().value_or(0.0);

using cache_size_pct_type = double;
static constexpr auto epsilon
= std::numeric_limits<cache_size_pct_type>::epsilon();

if ((cache_size_pct < epsilon) && (cloud_storage_cache_size() == 0)) {
Comment on lines +1982 to +1986
Copy link
Member

Choose a reason for hiding this comment

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

please add a commit that adds a comment explaining this. i read the docs for epsilon, but i don't see how it fits into this calculation

Copy link
Contributor Author

@WillemKauf WillemKauf Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For double floating-point comparison to 0- is there a different way you prefer doing this?

I can update the commit history to be clear.

Copy link
Member

Choose a reason for hiding this comment

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

I can update the commit history to be clear.
this already merged, so i think it'll have to be a comment?

return ss::format(
"Cannot set both {} and {} to 0.",
cloud_storage_cache_size.name(),
cloud_storage_cache_size_pct.name());
}

return std::nullopt;
}

} // namespace cloud_storage
11 changes: 11 additions & 0 deletions src/v/cloud_storage/cache_service.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "cloud_storage/access_time_tracker.h"
#include "cloud_storage/cache_probe.h"
#include "cloud_storage/recursive_directory_walker.h"
#include "config/configuration.h"
#include "config/property.h"
#include "resource_mgmt/io_priority.h"
#include "ssx/semaphore.h"
Expand Down Expand Up @@ -197,6 +198,16 @@ class cache : public ss::peering_sharded_service<cache> {
return _cache_dir / key;
}

// Checks if a cluster configuration is valid for the properties
// `cloud_storage_cache_size` and `cloud_storage_cache_size_percent`.
// Two cases are invalid: 1. the case in which both are 0, 2. the case in
// which `cache_size` is 0 while `cache_size_percent` is `std::nullopt`.
//
// Returns `std::nullopt` if the passed configuration is valid, or an
// `ss::sstring` explaining the misconfiguration otherwise.
static std::optional<ss::sstring>
validate_cache_config(const config::configuration& conf);

private:
/// Load access time tracker from file
ss::future<> load_access_time_tracker();
Expand Down