Skip to content
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

Add ledis cache support #16035

Closed
wants to merge 2 commits into from
Closed

Add ledis cache support #16035

wants to merge 2 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented May 31, 2021

This PR adds a new cache provider ledis which have been implemented in go-chi/cache but not supported.

For those Gitea instances which have lower memory but want to cache last commit information, it's a better case than memory or redis.

@lunny lunny added the type/enhancement An improvement of existing functionality label May 31, 2021
@lunny lunny mentioned this pull request May 31, 2021
6 tasks
@codecov-commenter
Copy link

codecov-commenter commented May 31, 2021

Codecov Report

Merging #16035 (1f14ba6) into main (fae07cb) will increase coverage by 0.00%.
The diff coverage is 60.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #16035   +/-   ##
=======================================
  Coverage   45.43%   45.43%           
=======================================
  Files         709      709           
  Lines       83686    83691    +5     
=======================================
+ Hits        38022    38028    +6     
- Misses      39564    39565    +1     
+ Partials     6100     6098    -2     
Impacted Files Coverage Δ
modules/setting/cache.go 44.44% <33.33%> (-2.44%) ⬇️
modules/cache/cache.go 38.55% <100.00%> (ø)
services/pull/temp_repo.go 26.59% <0.00%> (-3.20%) ⬇️
services/pull/check.go 25.51% <0.00%> (-2.76%) ⬇️
services/pull/patch.go 54.23% <0.00%> (-1.70%) ⬇️
models/repo_list.go 77.82% <0.00%> (+0.77%) ⬆️
modules/markup/html.go 79.15% <0.00%> (+1.04%) ⬆️
modules/queue/workerpool.go 55.34% <0.00%> (+1.52%) ⬆️
modules/process/manager.go 75.30% <0.00%> (+2.46%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fae07cb...1f14ba6. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 31, 2021
@silverwind
Copy link
Member

What is ledis? Some kind of redis fork?

@zeripath
Copy link
Contributor

I am slightly concerned that this is adding another different level dB to the system.

It would not be difficult to add a leveldb caching provider using our current leveldb by looking at this code nor should it be difficult to switch our leveldb provider over to the rdb that is in here but we shouldn't have two leveldbs.

Interestingly in the dependencies for this rdb is a mmap implementation - which (if we were happy to add - I think this is a cgo dependency) could be used in go-git for its commit-graph reader and some other places in gitea.

@lunny
Copy link
Member Author

lunny commented May 31, 2021

What is ledis? Some kind of redis fork?

It's a pure golang k/v database which have a redis similiar interface but stored all data into disk. It's based on go leveldb. So that's it's name level's l and edis from redis.

@zeripath The cache interface requires Incr and something other feature which needs some work based on leveldb. I don't think we need to create a new one since go-chi/cache have implemented that.
The cache will be removed when gitea restarted. Then we have to re-caculate the last commit info again when first visit the big repository. That's another place why a disk cache than memory.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 15, 2021
@techknowlogick techknowlogick added this to the 1.15.0 milestone Jun 15, 2021
@zeripath
Copy link
Contributor

I've just understood the reasoning behind this.

Is it really the case that the memory cache just grows without limit? That's kinda crazy.

Is there any limit on this cache?

@lunny
Copy link
Member Author

lunny commented Jun 22, 2021

I've just understood the reasoning behind this.

Is it really the case that the memory cache just grows without limit? That's kinda crazy.

Is there any limit on this cache?

If the cache haven't given a timeout when set, it will always be kept in memory when it's a memory provider. We also have redis implementation, but for a small instance, I think it's too overhead.

The memory cache will be cleared when Gitea restarted even if the cache item is not timeout. For a last commit info cache, it's bad.

@zeripath
Copy link
Contributor

Ok this absolutely needs fixing. I just don't understand why anyone would write a cache that's not size limited in some way...

Are the redis and ledis caches at least size limited in some way?

@lunny
Copy link
Member Author

lunny commented Jun 22, 2021

Ok this absolutely needs fixing. I just don't understand why anyone would write a cache that's not size limited in some way...

Are the redis and ledis caches at least size limited in some way?

The cache library derived from macaron middlewares and as I know there is no size limit for all these implementations.

@zeripath
Copy link
Contributor

o_O

Then this just swaps the problem of memory for one of disk space and is not much better.

@zeripath
Copy link
Contributor

Ok.

I think we need to change the memory one to be a wrapper around an lru. If you get a cache hit just check if it's already expired and delete it if so.

That's probably the simplest thing to do - as although that means that the cache may contain expired data that data is likely not to be hot anyway and is more likely to get thrown out in an lru cache.

There's probably a library out there that has this kind of wrapping already.

For the ledis and redis caches - we can't be the first to have to do this surely? There has to be a way of limiting these caches.

@zeripath
Copy link
Contributor

Ok redis has volatile-lru, volatile-ttl and allkeys-lru options. I think therefore Redis is a configuration issue for redis users - as long as our implementation is setting expires correctly.

(Likely we should not be recommending allkeys-lru if they're using redis for queues.)

We should check that sessions have an expire set on them too.

@6543
Copy link
Member

6543 commented Jul 4, 2021

Ledis lib looks stale as zeripath pointed out :/

@techknowlogick techknowlogick modified the milestones: 1.15.0, 1.16.0 Jul 13, 2021
@lunny lunny removed this from the 1.16.0 milestone Nov 9, 2021
@lunny lunny added the type/upstream This is an issue in one of Gitea's dependencies and should be reported there label Nov 9, 2021
@wxiaoguang
Copy link
Contributor

The ledis project seems not active anymore. Should we give it up?

@lunny lunny deleted the lunny/cache_ledis branch January 20, 2022 06:49
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. type/enhancement An improvement of existing functionality type/upstream This is an issue in one of Gitea's dependencies and should be reported there
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants