Expose Storage cache. #949
Conversation
ajnavarro
requested review from
jfontan,
erizocosmico,
smola and
mcuadros
and removed request for
mcuadros
storage/filesystem/object.go
Outdated
|
|
||
| return s, nil | ||
| func (s *ObjectStorage) WithCache(cache cache.Object) *ObjectStorage { | ||
| s.deltaBaseCache = cache |
There was a problem hiding this comment.
The cache should be passed to the constructor in the same way we do with the dotgit instance since is required, the options are optional, but if in the case of the cache the LRU is being instanced always in the constructor wasting resources.
func NewObjectStorage(dir *dotgit.DotGit, cache cache.Object) *ObjectStorage {
|
@mcuadros - now cache is required param in constructors. |
storage/filesystem/object.go
Outdated
| if oe != nil { | ||
| continue | ||
| } | ||
| o := NewObjectStorage(dg, cache.NewObjectLRUDefault()) |
There was a problem hiding this comment.
should we use here s.deltaBaseCache ?
There was a problem hiding this comment.
Ohh, good question - I think so.
Done (@ajnavarro )
Signed-off-by: kuba-- <[email protected]>
|
Oy gevalt! A backward incompatible change passed off in a minor release. Thanks! |
|
@maguro This is how you learn to use vendoring and |
|
Not sure why the dismissive tone, this is indeed a breaking change, and it should not have been merged. I've raised #1262. I think the signature change should be reverted. |
Signed-off-by: kuba-- [email protected]
If we want to let apps. adjust cache size (or pass own implementation) we need to expose it somehow.
It's related to src-d/gitbase#440
Also we don't need to return an error from constructors like
NewStorage.