node: Configure of the container cache size #1375

Open
achuprov wants to merge 1 commit from achuprov/frostfs-node:feat/add_container_cache_config into master
Member

Fetching the list of containers takes time. Let's add a container cache size setting

Signed-off-by: Alexander Chuprov a.chuprov@yadro.com

Fetching the list of containers takes time. Let's add a container cache size setting Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
achuprov added 1 commit 2024-09-13 09:13:03 +00:00
[#1375] node: Configure of the container cache size
All checks were successful
DCO action / DCO (pull_request) Successful in 1m43s
Vulncheck / Vulncheck (pull_request) Successful in 2m8s
Pre-commit hooks / Pre-commit (pull_request) Successful in 2m21s
Tests and linters / Run gofumpt (pull_request) Successful in 2m38s
Tests and linters / gopls check (pull_request) Successful in 2m43s
Build / Build Components (pull_request) Successful in 2m59s
Tests and linters / Staticcheck (pull_request) Successful in 2m58s
Tests and linters / Lint (pull_request) Successful in 3m37s
Tests and linters / Tests (pull_request) Successful in 5m30s
Tests and linters / Tests with -race (pull_request) Successful in 5m33s
fa0480a3b6
Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
achuprov force-pushed feat/add_container_cache_config from fa0480a3b6 to 67acdb6874 2024-09-13 09:13:48 +00:00 Compare
fyrchik requested review from storage-core-committers 2024-09-13 09:28:09 +00:00
fyrchik requested review from storage-core-developers 2024-09-13 09:28:10 +00:00
fyrchik added the
config
label 2024-09-13 09:28:16 +00:00
fyrchik added the
frostfs-ir
frostfs-node
labels 2024-09-13 09:30:15 +00:00
fyrchik requested changes 2024-09-13 09:43:12 +00:00
@ -80,3 +80,3 @@
}
if c.cfgMorph.cacheTTL <= 0 {
if c.cfgMorph.cacheTTL <= 0 || c.cfgMorph.containerCacheSize == 0 {
Owner

It seems wrong, the value of containerCacheSize should not affect other caches.

It seems wrong, the value of `containerCacheSize` should not affect other caches.
Author
Member

fixed

fixed
Owner

What is the behaviour for containerCacheSize == 0 now?

What is the behaviour for `containerCacheSize == 0` now?
Author
Member

Now the behavior is similar c.cfgMorph.cacheTTL=0 for cnrSource

Now the behavior is similar `c.cfgMorph.cacheTTL=0` for `cnrSource`
@ -80,6 +80,7 @@ morph:
cache_ttl: 15s # Sidechain cache TTL value (min interval between similar calls). Negative value disables caching.
# Default value: block time. It is recommended to have this value less or equal to block time.
# Cached entities: containers, container lists, eACL tables.
container_cache_size: 1000 # container_cache_size is responsible for the number of containers that are in the cache.
Owner

is responsible for the number -> is the maximum number

`is responsible for the number` -> `is the maximum number`
achuprov marked this conversation as resolved
realloc reviewed 2024-09-13 09:58:38 +00:00
@ -31,2 +31,4 @@
// FrostfsIDCacheSizeDefault is a default value of APE chain cache.
FrostfsIDCacheSizeDefault = 10_000
// ContainerCacheSizeDefault is a default value of size container cache.
Owner

// ContainerCacheSizeDefault represents the default size for the container cache.

// ContainerCacheSizeDefault represents the default size for the container cache.
achuprov marked this conversation as resolved
dstepanov-yadro reviewed 2024-09-13 10:29:44 +00:00
@ -32,1 +32,4 @@
FrostfsIDCacheSizeDefault = 10_000
// ContainerCacheSizeDefault is a default value of size container cache.
ContainerCacheSizeDefault = 100

Is there any estimate on the size of the container? Maybe 100 is a very small (or vice versa, a very large) value?

Is there any estimate on the size of the container? Maybe 100 is a very small (or vice versa, a very large) value?
Owner

100 is as it was before, seems like a sane default to me.

100 is as it was before, seems like a sane default to me.
achuprov force-pushed feat/add_container_cache_config from 67acdb6874 to c63f4d6ef5 2024-09-13 13:05:18 +00:00 Compare
achuprov force-pushed feat/add_container_cache_config from c63f4d6ef5 to 3052d113e8 2024-09-13 13:09:02 +00:00 Compare
Member

Can you please add some comments to the PR description and to the commit message on what this PR is about?

Can you please add some comments to the PR description and to the commit message on what this PR is about?
achuprov force-pushed feat/add_container_cache_config from 3052d113e8 to 24897daacb 2024-09-24 14:15:51 +00:00 Compare
achuprov force-pushed feat/add_container_cache_config from 24897daacb to 549b1fe2d5 2024-09-24 14:16:31 +00:00 Compare
achuprov force-pushed feat/add_container_cache_config from 549b1fe2d5 to 97ce5ed4a3 2024-09-24 15:09:17 +00:00 Compare
achuprov force-pushed feat/add_container_cache_config from 97ce5ed4a3 to be52657175 2024-09-24 15:15:34 +00:00 Compare
achuprov requested review from storage-core-committers 2024-09-24 15:18:12 +00:00
acid-ant requested changes 2024-09-25 05:51:02 +00:00
Dismissed
@ -90,2 +89,2 @@
cachedContainerStorage := newCachedContainerStorage(cnrSrc, c.cfgMorph.cacheTTL)
cachedEACLStorage := newCachedEACLStorage(eACLFetcher, c.cfgMorph.cacheTTL)
cachedContainerStorage := cnrSrc
if c.cfgMorph.containerCacheSize > 0 {
Member

Why this line added?

Why this line added?
Member

@acid-ant is right. If we add this check, then we must be able to support backward-combability with already deployed systems - we'll accidently disable container caching. Of course, we can fixate this parameter in config for frostfs-storage update but the way simpler is to introduce default value for containerCacheSize

@acid-ant is right. If we add this check, then we must be able to support backward-combability with already deployed systems - we'll accidently disable container caching. Of course, we can fixate this parameter in config for frostfs-storage update but the way simpler is to introduce default value for `containerCacheSize`
Author
Member

@aarifullin We have the constant ContainerCacheSizeDefault=100. Therefore, the default behavior will not change.

@aarifullin We have the constant `ContainerCacheSizeDefault=100`. Therefore, the default behavior will not change.
Author
Member

@acid-ant We don't need create a subscribers if the containerCache is disabled.

@acid-ant We don't need create a subscribers if the `containerCache` is disabled.
acid-ant marked this conversation as resolved
@ -92,0 +89,4 @@
cachedContainerStorage := cnrSrc
if c.cfgMorph.containerCacheSize > 0 {
// use RPC node as source of Container contract items (with caching)
cachedContainerStorage := newCachedContainerStorage(cnrSrc, c.cfgMorph.cacheTTL, c.cfgMorph.containerCacheSize)
Member

You've created cachedContainerStorage but don't use it for c.cfgObject.cnrSource. Is it intentional?

You've created `cachedContainerStorage` but don't use it for `c.cfgObject.cnrSource`. Is it intentional?
Author
Member

fixed typo

fixed typo
acid-ant marked this conversation as resolved
achuprov force-pushed feat/add_container_cache_config from be52657175 to 4c77e17b4f 2024-09-27 09:40:19 +00:00 Compare
achuprov force-pushed feat/add_container_cache_config from 4c77e17b4f to 81c63a8cf2 2024-09-27 09:41:11 +00:00 Compare
acid-ant approved these changes 2024-09-27 11:57:36 +00:00
All checks were successful
Pre-commit hooks / Pre-commit (pull_request) Successful in 3m13s
Required
Details
Tests and linters / Run gofumpt (pull_request) Successful in 3m12s
Required
Details
DCO action / DCO (pull_request) Successful in 3m38s
Required
Details
Build / Build Components (pull_request) Successful in 3m35s
Required
Details
Vulncheck / Vulncheck (pull_request) Successful in 3m33s
Required
Details
Tests and linters / Staticcheck (pull_request) Successful in 5m33s
Required
Details
Tests and linters / gopls check (pull_request) Successful in 5m44s
Required
Details
Tests and linters / Lint (pull_request) Successful in 6m1s
Required
Details
Tests and linters / Tests (pull_request) Successful in 6m52s
Required
Details
Tests and linters / Tests with -race (pull_request) Successful in 7m46s
Required
Details
This pull request doesn't have enough approvals yet. 1 of 2 approvals granted.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u feat/add_container_cache_config:achuprov-feat/add_container_cache_config
git checkout achuprov-feat/add_container_cache_config
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-developers
No milestone
No project
No assignees
7 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#1375
No description provided.