TrueCloudLab/frostfs-node
21
1
Fork 27
Code Issues 108 Pull requests 3 Projects Releases 1 Activity Actions

node: Configure of the container cache size #1375

Merged
fyrchik merged 1 commit from achuprov/frostfs-node:feat/add_container_cache_config into master 2024-10-01 14:05:53 +00:00
Conversation 1 Commits 1 Files changed 6 +55 -35
achuprov commented 2024-09-13 09:13:03 +00:00
Member
Copy link

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 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
Dismissed
cmd/frostfs-node/container.go Outdated
@ -80,3 +80,3 @@
}
if c.cfgMorph.cacheTTL <= 0 {
if c.cfgMorph.cacheTTL <= 0 || c.cfgMorph.containerCacheSize == 0 {
fyrchik commented 2024-09-13 09:40:44 +00:00
Owner
Copy link

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

It seems wrong, the value of `containerCacheSize` should not affect other caches.
achuprov commented 2024-09-13 14:40:40 +00:00
Author
Member
Copy link

fixed

fixed
fyrchik commented 2024-09-13 15:12:10 +00:00
Owner
Copy link

What is the behaviour for containerCacheSize == 0 now?

What is the behaviour for `containerCacheSize == 0` now?
achuprov commented 2024-09-24 15:35:25 +00:00
Author
Member
Copy link

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

Now the behavior is similar `c.cfgMorph.cacheTTL=0` for `cnrSource`
config/example/node.yaml Outdated
@ -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.
fyrchik commented 2024-09-13 09:41:22 +00:00
Owner
Copy link

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
cmd/frostfs-node/config/morph/config.go Outdated
@ -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.
realloc commented 2024-09-13 09:58:38 +00:00
Owner
Copy link

// 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
cmd/frostfs-node/config/morph/config.go Outdated
@ -32,1 +32,4 @@
FrostfsIDCacheSizeDefault = 10_000
// ContainerCacheSizeDefault is a default value of size container cache.
ContainerCacheSizeDefault = 100
dstepanov-yadro commented 2024-09-13 10:29:44 +00:00
Member
Copy link

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?
fyrchik commented 2024-09-13 11:54:02 +00:00
Owner
Copy link

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
elebedeva commented 2024-09-13 15:48:28 +00:00
Member
Copy link

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?
👍 1
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
cmd/frostfs-node/container.go Outdated
@ -90,2 +89,2 @@
cachedContainerStorage := newCachedContainerStorage(cnrSrc, c.cfgMorph.cacheTTL)
cachedEACLStorage := newCachedEACLStorage(eACLFetcher, c.cfgMorph.cacheTTL)
cachedContainerStorage := cnrSrc
if c.cfgMorph.containerCacheSize > 0 {
acid-ant commented 2024-09-25 05:49:26 +00:00
Member
Copy link

Why this line added?

Why this line added?
aarifullin commented 2024-09-25 13:23:35 +00:00
Member
Copy link

@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`
achuprov commented 2024-09-27 09:23:19 +00:00
Author
Member
Copy link

@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.
achuprov commented 2024-09-27 09:28:06 +00:00
Author
Member
Copy link

@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
cmd/frostfs-node/container.go Outdated
@ -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)
acid-ant commented 2024-09-25 05:50:56 +00:00
Member
Copy link

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?
achuprov commented 2024-09-27 09:40:44 +00:00
Author
Member
Copy link

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
aarifullin approved these changes 2024-09-30 08:16:52 +00:00
fyrchik approved these changes 2024-10-01 14:05:47 +00:00
fyrchik merged commit a13219808a into master 2024-10-01 14:05:53 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
acid-ant
aarifullin
fyrchik
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
badger

Badger DB related issues

  
frostfs-adm

   
frostfs-cli

   
frostfs-ir

   
frostfs-lens

   
frostfs-node

   
good first issue

Good for newcomers

  
triage
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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.
Delete branch "achuprov/frostfs-node:feat/add_container_cache_config"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?