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

improve redis cache #5137

Closed
wkloucek opened this issue Nov 28, 2022 · 8 comments
Closed

improve redis cache #5137

wkloucek opened this issue Nov 28, 2022 · 8 comments

Comments

@wkloucek
Copy link
Contributor

wkloucek commented Nov 28, 2022

Cache backend availability

current state

If the cache backend is not available (eg. Redis down / away /...), oCIS will respond with HTTP status 500 to a lot of requests (generally requests that want to query / write to the cache).

-> cache backend unavailable = dead oCIS

proposed state

This cache is kind of optional, because we could still answer requests if the cache backend is not responsive. Downside is that requests would take longer and there is a risk of producing high load (that the system can't bear). But the cache would no longer need to promise a 99,9999..% availability.

-> cache backend unavailable = slow oCIS

List operation

we should only list from a database (https://github.com/cs3org/reva/blob/11cc78add8ba06a70c0515aa762f07d1ebd65ab7/pkg/storage/cache/cache.go#L197-L200, https://github.com/go-micro/plugins/blob/c91295de81c390c8152776186c4f6747da9f2bbc/v4/store/redis/redis.go#L123-L126) because KEYS * is too expensive

The list operation also needs to remove the database prefix it adds during Write https://github.com/go-micro/plugins/blob/c91295de81c390c8152776186c4f6747da9f2bbc/v4/store/redis/redis.go#L111, making the workaround cs3org/reva#3338 uneeded.

Misc

From my POV it would be nice to separate the table name from the key name by eg using :. This would lead to some more readable redis keys like mytable:some-item1

REVA

using KEYS * (see above) or KEYS table* triggered by https://github.com/cs3org/reva/blob/edge/pkg/storage/cache/stat.go#L53 is too expensive in Redis.
We should transfer the string.Contains logic https://github.com/cs3org/reva/blob/11cc78add8ba06a70c0515aa762f07d1ebd65ab7/pkg/storage/cache/stat.go#L59-L72 into intelligent redis KEYS queries. Redis supports patterns https://redis.io/commands/keys/

Originally posted by @wkloucek in #4794 (comment)

@jvillafanez
Copy link
Member

In general, listing should be avoided in any type of cache.
I guess most of the caches use a hash map in some way to provide read and write access in constant time, but the keys will be unordered. This means that we'll have to check all the keys to find something like "table*".
There might be other caches that use a radix tree or a tree like structure (likely etcd), which can provide prefix searches, but write operations will be slower.

If we still have to implement listing in redis, SCAN is the way to go. As the doc warning says, KEYS will block other requests, so if the cache is too large it will severely affect other requests and consequently the performance of the whole system. Note that it doesn't matter if there are only 2 results that could match KEYS abcde*, it will still need to go through the whole cache.
I guess that SCAN will return a subset of the results in a fixed time, so at least the other requests won't be blocked for too long. This won't hinder the performance of the rest of the system. SCAN is expected to perform worse though due to the amount of requests you'll have to do to get the results.

@IljaN
Copy link
Member

IljaN commented Dec 1, 2022

Not sure if it helps in this case but it is possible to manually build a index using redis sets. (SADD/SINTER)
Members of the Set are the keys (Add them with SADD) and you can search the set using SINTER (Set Intersection)

https://redis.io/commands/sadd/
https://redis.io/commands/sinter/

@jvillafanez
Copy link
Member

I'd rather not using SINTER.

  • I assume we'd need to deal with millions, billions or even a higher number of keys. We'd need to store all that data in a set; I don't know what are the limits, but it seems too much.
  • Memory usage will increase, and it's duplicated information (keys are there)
  • The best scenario is that the set is hashed so the intersection doesn't need to traverse the whole set, just the smaller one
  • Prefix search, which I think is what we want, isn't possible

@wkloucek
Copy link
Contributor Author

wkloucek commented Feb 3, 2023

Added the section "Cache backend availability"

@butonic
Copy link
Member

butonic commented Mar 23, 2023

I created an upstream PR to allow using List with prefix and suffix so we can implement cache invalidation a little more efficiently: micro/plugins#97

@wkloucek
Copy link
Contributor Author

@butonic do you have an overview about missing improvements of the redis cache? Can we close this issue?

@individual-it
Copy link
Member

Not sure if this is related but if the system is already a bit busy and I upload images through the webUI I often get 500 errors when it tries to generate the previews.

<?xml version="1.0" encoding="UTF-8"?>
<d:error xmlns:d="DAV" xmlns:s="http://sabredav.org/ns"><s:exception></s:exception><s:message>{&#34;id&#34;:&#34;go.micro.client&#34;,&#34;code&#34;:408,&#34;detail&#34;:&#34;context deadline exceeded&#34;,&#34;status&#34;:&#34;Request Timeout&#34;}</s:message></d:error>

log

{"level":"info","service":"proxy","proto":"HTTP/1.0","request-id":"127e4f2f-a0cb-45ff-ba04-54af6409d731","remote-addr":"172.25.0.1:49548","method":"GET","status":500,"path":"/remote.php/dav/spaces/e89c7a91-5166-4516-ba2b-c84cd130bf62$007f0f4e-cb0c-4d60-a229-180920aa97ec/memory-usage-and-image-preview/P1100055.JPG","duration":5202.13825,"bytes":310,"time":"2023-07-04T09:21:32.878352469Z","line":"github.com/owncloud/ocis/v2/services/proxy/pkg/middleware/accesslog.go:28","message":"access-log"}
{"level":"info","service":"proxy","proto":"HTTP/1.0","request-id":"127e4f2f-a0cb-45ff-ba04-54af6409d731","remote-addr":"172.25.0.1:49536","method":"GET","status":500,"path":"/remote.php/dav/spaces/e89c7a91-5166-4516-ba2b-c84cd130bf62$007f0f4e-cb0c-4d60-a229-180920aa97ec/memory-usage-and-image-preview/P1070231.JPG","duration":5273.255582,"bytes":310,"time":"2023-07-04T09:21:32.94923889Z","line":"github.com/owncloud/ocis/v2/services/proxy/pkg/middleware/accesslog.go:28","message":"access-log"}

@wkloucek
Copy link
Contributor Author

Closing because this ticket does not reflect the current state. All / most issues should be resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants