mo-service: fix default cache configs

[reusee]

What type of PR is this?

• API-change
• BUG
• Improvement
• Documentation
• Feature
• Test and CI
• Code Refactoring

Which issue(s) this PR fixes:

issue #18688

What this PR does / why we need it:

fileservice: do not set default memory and disk config in setDefaults

[codiumai-pr-agent-pro]

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review Potential Inconsistency The default memory capacity for LocalFileService and SharedFileService is set to 512MB, but this value is not used for S3FileService. This might lead to inconsistent behavior across different file service types. Possible Bug The enableDiskCacheForLocalFS flag is still being used, but its purpose is unclear given the new cache configuration approach. This might lead to unexpected behavior if not properly handled.

[codiumai-pr-agent-pro]

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Code Suggestions ✨

Category	Suggestion	Score
Maintainability	Extract memory cache initialization into a separate function for better code organization Consider extracting the memory cache initialization logic into a separate function to improve code readability and maintainability. pkg/fileservice/local_fs.go [135-147] // memory -if config.MemoryCapacity != nil && - *config.MemoryCapacity > DisableCacheCapacity { // 1 means disable - l.memCache = NewMemCache( - fscache.ConstCapacity(int64(*config.MemoryCapacity)), - &config.CacheCallbacks, - l.perfCounterSets, - ) - logutil.Info("fileservice: memory cache initialized", - zap.Any("fs-name", l.name), - zap.Any("config", config), - ) +if err := l.initMemoryCache(config); err != nil { + return err } +// ... rest of the function ... + +func (l *LocalFS) initMemoryCache(config CacheConfig) error { + if config.MemoryCapacity != nil && *config.MemoryCapacity > DisableCacheCapacity { + l.memCache = NewMemCache( + fscache.ConstCapacity(int64(*config.MemoryCapacity)), + &config.CacheCallbacks, + l.perfCounterSets, + ) + logutil.Info("fileservice: memory cache initialized", + zap.Any("fs-name", l.name), + zap.Any("config", config), + ) + } + return nil +} + Suggestion importance[1-10]: 8 Why: This suggestion enhances code readability and maintainability by isolating the memory cache initialization logic, making the main function cleaner and easier to understand.	8
	Extract memory and disk cache initialization into separate functions to reduce code duplication Consider extracting the common logic for initializing memory and disk caches into separate functions to reduce code duplication and improve maintainability. pkg/fileservice/s3_fs.go [159-180] -// memory cache -if config.MemoryCapacity != nil && - *config.MemoryCapacity > DisableCacheCapacity { - s.memCache = NewMemCache( - fscache.ConstCapacity(int64(*config.MemoryCapacity)), - &config.CacheCallbacks, - s.perfCounterSets, - ) - logutil.Info("fileservice: memory cache initialized", - zap.Any("fs-name", s.name), - zap.Any("capacity", config.MemoryCapacity), - ) +if err := s.initMemoryCache(config); err != nil { + return err } -// disk cache -if config.DiskCapacity != nil && - *config.DiskCapacity > DisableCacheCapacity && - config.DiskPath != nil { - var err error - s.diskCache, err = NewDiskCache( - ctx, - *config.DiskPath, +if err := s.initDiskCache(ctx, config); err != nil { + return err +} +// ... rest of the function ... + +func (s *S3FS) initMemoryCache(config CacheConfig) error { + if config.MemoryCapacity != nil && *config.MemoryCapacity > DisableCacheCapacity { + s.memCache = NewMemCache( + fscache.ConstCapacity(int64(*config.MemoryCapacity)), + &config.CacheCallbacks, + s.perfCounterSets, + ) + logutil.Info("fileservice: memory cache initialized", + zap.Any("fs-name", s.name), + zap.Any("capacity", config.MemoryCapacity), + ) + } + return nil +} + +func (s *S3FS) initDiskCache(ctx context.Context, config CacheConfig) error { + if config.DiskCapacity != nil && *config.DiskCapacity > DisableCacheCapacity && config.DiskPath != nil { + var err error + s.diskCache, err = NewDiskCache( + ctx, + *config.DiskPath, + // ... rest of the NewDiskCache parameters ... + ) + if err != nil { + return err + } + logutil.Info("fileservice: disk cache initialized", + // ... log parameters ... + ) + } + return nil +} + Suggestion importance[1-10]: 8 Why: This suggestion significantly enhances maintainability by reducing code duplication and improving the organization of the cache initialization logic, making future modifications easier.	8
Enhancement	Refactor the cache configuration logic to use a more concise switch statement Consider using a switch statement instead of multiple if-else conditions for setting default cache configurations. This can improve readability and maintainability of the code. cmd/mo-service/config.go [664-699] for i := 0; i < len(c.FileServices); i++ { config := &c.FileServices[i] // cache configs switch config.Name { - - case defines.LocalFileServiceName: - // memory + case defines.LocalFileServiceName, defines.SharedFileServiceName: if config.Cache.MemoryCapacity == nil { capacity := tomlutil.ByteSize(512 * (1 << 20)) config.Cache.MemoryCapacity = &capacity } - // no disk - - case defines.SharedFileServiceName: - // memory - if config.Cache.MemoryCapacity == nil { - capacity := tomlutil.ByteSize(512 * (1 << 20)) - config.Cache.MemoryCapacity = &capacity + if config.Name == defines.SharedFileServiceName { + if config.Cache.DiskPath == nil { + path := config.DataDir + "-cache" + config.Cache.DiskPath = &path + } + if config.Cache.DiskCapacity == nil { + capacity := tomlutil.ByteSize(8 * (1 << 30)) + config.Cache.DiskCapacity = &capacity + } } - // disk - if config.Cache.DiskPath == nil { - path := config.DataDir + "-cache" - config.Cache.DiskPath = &path - } - if config.Cache.DiskCapacity == nil { - capacity := tomlutil.ByteSize(8 * (1 << 30)) - config.Cache.DiskCapacity = &capacity - } - case defines.ETLFileServiceName: // no caches - } Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion improves code readability and maintainability by consolidating similar logic into a single case, reducing redundancy. However, the improvement is not critical, as the existing code is already clear.	7
Best practice	Replace magic number with a named constant for better code clarity *Consider using a constant for the magic number '1' in the condition config.MemoryCapacity > DisableCacheCapacity. This will improve code readability and maintainability. pkg/fileservice/local_fs.go [136-137] +const MinCacheCapacity = 1 if config.MemoryCapacity != nil && - *config.MemoryCapacity > DisableCacheCapacity { // 1 means disable + *config.MemoryCapacity > MinCacheCapacity { Apply this suggestion Suggestion importance[1-10]: 6 Why: Using a named constant instead of a magic number improves code clarity and maintainability, but the impact is minor as the comment already explains the number's purpose.	6