-
Notifications
You must be signed in to change notification settings - Fork 360
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.
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
Configurable repository and commit cache #4910
Conversation
Enable lakefs.yaml cache config for graveler's repository and commit cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THANKS!
pkg/config/config.go
Outdated
@@ -449,3 +450,19 @@ func (c *Config) GetUIEnabled() bool { | |||
func (c *Config) GetLoginDuration() time.Duration { | |||
return c.values.Auth.LoginDuration | |||
} | |||
|
|||
func (c *Config) GravelerRepositoryCacheConfig() ref.CacheConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func (c *Config) GravelerRepositoryCacheConfig() ref.CacheConfig { | |
func (c *Config) GetGravelerRepositoryCacheParams() ref.CacheConfig { |
for consistency, e.g. like GetBlockAdapterS3Params. Also below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - I'll address the new/old practice in a different pr
pkg/config/defaults.go
Outdated
GravelerRepositoryCacheJitterKey = "graveler.repository_cache.jitter" | ||
DefaultGravelerRepositoryCacheJitter = DefaultGravelerRepositoryCacheExpiry / 2 | ||
GravelerCommitCacheSizeKey = "graveler.commit_cache.size" | ||
DefaultGravelerCommitCacheSize = 50000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
DefaultGravelerCommitCacheSize = 50000 | |
DefaultGravelerCommitCacheSize = 50_000 |
:-)
pkg/config/defaults.go
Outdated
GravelerCommitCacheExpiryKey = "graveler.commit_cache.expiry" | ||
DefaultGravelerCommitCacheExpiry = 30 * time.Second | ||
GravelerCommitCacheJitterKey = "graveler.commit_cache.jitter" | ||
DefaultGravelerCommitCacheJitter = DefaultGravelerCommitCacheExpiry / 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems very high, we want a long expiry time. And if it's inconsistent, FindMergeBase may take very different times to do the exact same thing.
DefaultGravelerCommitCacheJitter = DefaultGravelerCommitCacheExpiry / 2 | |
DefaultGravelerCommitCacheJitter = 2 * time.Second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree - I also not sure jitter is needed for these type of cache.
I'll update both to 2 seconds
pkg/config/defaults.go
Outdated
GravelerCommitCacheSizeKey = "graveler.commit_cache.size" | ||
DefaultGravelerCommitCacheSize = 50000 | ||
GravelerCommitCacheExpiryKey = "graveler.commit_cache.expiry" | ||
DefaultGravelerCommitCacheExpiry = 30 * time.Second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice... but even this seems low! What's the harm in keeping something in the commit cache? Even 5 minutes seems reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No harm - updated the size and forgot to update the expiry. Setting to 10min - comment if you think we should go higher.
Close #4637