Cache commit on Graveler ref manager #4497
Conversation
nopcoder
added
the
include-changelog
pkg/graveler/ref/manager.go
Outdated
| @@ -31,19 +31,24 @@ const ( | |||
| DefaultRepositoryCacheSize = 1000 | |||
| DefaultRepositoryCacheExpiry = 5 * time.Second | |||
| DefaultRepositoryCacheJitter = DefaultRepositoryCacheExpiry / 2 | |||
|
|
|||
| DefaultCommitCacheSize = 1000 | |||
| DefaultCommitCacheExpiry = 5 * time.Second | |||
There was a problem hiding this comment.
Let's increase it to 30 seconds? It's immutable anyhow..
pkg/graveler/ref/manager.go
Outdated
| // commits cache | ||
| commitCacheConfig := cfg.CommitCacheConfig | ||
| if commitCacheConfig == nil { | ||
| commitCacheConfig = &CacheConfig{ | ||
| Size: DefaultCommitCacheSize, | ||
| Expiry: DefaultCommitCacheExpiry, | ||
| Jitter: DefaultCommitCacheJitter, | ||
| } | ||
| } | ||
| commitCache := cache.NoCache | ||
| if commitCacheConfig.Size > 0 { | ||
| commitCache = cache.NewCache(commitCacheConfig.Size, commitCacheConfig.Expiry, cache.NewJitterFn(commitCacheConfig.Jitter)) | ||
| } |
There was a problem hiding this comment.
Minor: Duplicated logic to shared location
| @@ -456,23 +487,38 @@ func (m *KVManager) GetCommitByPrefix(ctx context.Context, repository *graveler. | |||
| } | |||
|
|
|||
| func (m *KVManager) GetCommit(ctx context.Context, repository *graveler.RepositoryRecord, commitID graveler.CommitID) (*graveler.Commit, error) { | |||
| key := fmt.Sprintf("%s:%s", repository.RepositoryID, commitID) | |||
| v, err := m.repoCache.GetOrSet(key, func() (v interface{}, err error) { | |||
| return m.getCommitBatch(ctx, repository, commitID) | |||
There was a problem hiding this comment.
It won't cache NotFound commits. I can't remember if we expect this to happen a lot. The chances of a user pointing directly to a commit hash which wasn't created yet is not big. However, we accept refs which could be anything. If we currently check that main isn't a commit, I think we should avoid that constant call to the store.
There was a problem hiding this comment.
I think that full commit ID for non existing is something that probably will not be part of any main flow. Anything short or resolved from branch or tag should be an existing commit.
So filling the cache with non existing commit for a case that a user/app try to identify if commit exits I assume it will be something I don't want to return answer from the last 30sec.
Keeping it this way.
There was a problem hiding this comment.
Will open an issue/pr to handle optimization of branch vs commit prefix.
pkg/graveler/ref/manager_test.go
Outdated
| ctx := context.Background() | ||
| mockStore.EXPECT().Get(ctx, []byte("graveler"), []byte("repos/repo1")).Times(times).Return(&kv.ValueWithPredicate{}, nil) | ||
| repoCacheConfig := &ref.RepositoryCacheConfig{ | ||
| cacheConfig := &ref.CacheConfig{ | ||
| Size: 100, | ||
| Expiry: 2 * time.Second, |
There was a problem hiding this comment.
If we're going to sleep in tests, why not 20 milliseconds?
nopcoder
force-pushed
the
feature/cache-commits
branch
from
ee37ea5 to
7bf4ecf
Compare
Close #4422