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

Multilevel/Redis cache backend #87

Open
wants to merge 34 commits into
base: master
from

Conversation

@jochen42
Copy link

jochen42 commented Nov 10, 2019

A simple implementation of an MultiLevel-Cache-Backends and an Redis-Cache-Backend.

The redisBackend can be used as a second-level cache together with the inMemoryBackend to reduced the pressure to http-backends in case of large traffic-increase in a short time-period.

The RedisBackend has a simple Lock-Mechanism which prevents parallel writes of the same cache-key.

Another idea is to creata an awsS3 Backend. This will remove the need of manage an additional redis-instance. It is a little slower than redis, but scales really well.

Additional i created a common backend test case, which can tests the basic functionality of all implemented backends.

This is a first concept. Implementation to be discussed. I will add some documentation the next days.

core/cache/redisBackend.go Outdated Show resolved Hide resolved
core/cache/redisBackend.go Outdated Show resolved Hide resolved
core/cache/redisBackend_test.go Outdated Show resolved Hide resolved
@jochen42 jochen42 force-pushed the jochen42:redis-cache-backend branch 10 times, most recently from c200fc7 to 2e7e3b9 Nov 10, 2019
core/cache/backendHitCounter.go Outdated Show resolved Hide resolved
type (
// BackendHitCounter representation
BackendHitCounter struct {
backendType string

This comment has been minimized.

Copy link
@danielpoe

danielpoe Nov 11, 2019

Member

thinking of adding another tag - cacheName ?
Different cache frontends use same backend.

This comment has been minimized.

Copy link
@jochen42

jochen42 Nov 11, 2019

Author

what do you expect as an value for the cacheName?

core/cache/backendTestCase_test.go Outdated Show resolved Hide resolved
core/cache/inMemoryBackend_test.go Outdated Show resolved Hide resolved
core/cache/multiLevelBackend.go Outdated Show resolved Hide resolved
core/cache/redisBackend.go Outdated Show resolved Hide resolved
core/cache/redisBackend.go Show resolved Hide resolved
core/cache/redisBackend.go Outdated Show resolved Hide resolved
core/cache/redisBackend.go Show resolved Hide resolved
core/cache/redisBackend.go Outdated Show resolved Hide resolved
core/cache/redisBackend.go Outdated Show resolved Hide resolved
core/cache/fileBackend.go Outdated Show resolved Hide resolved
@jochen42 jochen42 force-pushed the jochen42:redis-cache-backend branch 7 times, most recently from 52f0e18 to fe0b40a Nov 11, 2019
Copy link
Contributor

bastianccm left a comment

Can we split this PR and move the general cache stuff (Metrics etc) into a new PR? This one can stay for the redis stuff I think?
This one gets too big for a proper review.

@@ -28,6 +28,8 @@ type (
Purge(key string) error
PurgeTags(tags []string) error
Flush() error
TagSupport() bool

This comment has been minimized.

Copy link
@bastianccm

bastianccm Nov 14, 2019

Contributor

+1 on type assertions. Do not change an existing, exposed interface (backwards incompatible)

core/cache/cache.go Outdated Show resolved Hide resolved
@danielpoe

This comment has been minimized.

Copy link
Member

danielpoe commented Nov 18, 2019

Can we split this PR and move the general cache stuff (Metrics etc) into a new PR? This one can stay for the redis stuff I think?
This one gets too big for a proper review.

I wouldn't split this now - not worth the extra effort separating everything again.
Lets do proper QA/ Review on that one and finish the feature completly.
The complete cache modul benefits from it and was in a very initial state anyhow

core/cache/fileBackend.go Outdated Show resolved Hide resolved
core/cache/multiLevelBackend.go Outdated Show resolved Hide resolved
core/cache/multiLevelBackend.go Outdated Show resolved Hide resolved
@jochen42 jochen42 force-pushed the jochen42:redis-cache-backend branch from 915f400 to 6df3a6e Nov 19, 2019
@jochen42

This comment has been minimized.

Copy link
Author

jochen42 commented Nov 20, 2019

@danielpoe @bastianccm

it seems, that the httpFrontend caches also error-responses. this is dangerous with an shared cache-backend. i would suggest to extend the frontend to cache only 4xx-error response, but not 5xx.

what do you think?

@danielpoe

This comment has been minimized.

Copy link
Member

danielpoe commented Nov 20, 2019

@danielpoe @bastianccm

it seems, that the httpFrontend caches also error-responses. this is dangerous with an shared cache-backend. i would suggest to extend the frontend to cache only 4xx-error response, but not 5xx.

what do you think?

Thats on purpose - if the backend responses with an error you dont need to retry.
But the cache time is much shorter..
Its a kind of "circuit breaker"

@jochen42

This comment has been minimized.

Copy link
Author

jochen42 commented Nov 20, 2019

@danielpoe @bastianccm
it seems, that the httpFrontend caches also error-responses. this is dangerous with an shared cache-backend. i would suggest to extend the frontend to cache only 4xx-error response, but not 5xx.
what do you think?

Thats on purpose - if the backend responses with an error you dont need to retry.
But the cache time is much shorter..
Its a kind of "circuit breaker"

ok. i am not a fan of this, because in kubernetes we would break the circuit to a service-address not a pod. if the backend-service has multiple pods, no retry on an other backend-pod will be done. and with the shared cache, the error will be cached in all flamingo-pods.

the circuit should be done at another place. perhaps we can make this frontend-behavior configurable?

@danielpoe

This comment has been minimized.

Copy link
Member

danielpoe commented Dec 4, 2019

Comments from Meeting 4.12.2019:

  1. Multilevel Backend -> Adjust to TwoLevelBackend with a Slow and a Fast backend.
    Misses in Fast and Hit in Slow should update Fast.

  2. Remove useless current Locking in Redis Backend

  3. Simple CacheFactory stuff

Addon Required: Think about a shared Lock: Because now cache entries expired still lead to thundering because all flamingo pods will try to refetch expired cache entries
=> New PR

Jochen Weber added 2 commits Dec 5, 2019
Jochen Weber
Jochen Weber
core/cache/redisBackend.go Outdated Show resolved Hide resolved
core/cache/redisBackend.go Outdated Show resolved Hide resolved
Jochen Weber added 4 commits Dec 5, 2019
Jochen Weber
Jochen Weber
Jochen Weber
_
Jochen Weber
core/cache/redisBackend.go Outdated Show resolved Hide resolved
@jochen42

This comment has been minimized.

Copy link
Author

jochen42 commented Dec 5, 2019

@danielpoe @bastianccm

i did some rework we talked about. open point's are

  • the new name CacheMetrics causes an linting error.
  • CacheFactory (i only heard the last minute of your plans. either write down your plan here or somebody else can implement)
  • handling frontendName using dingo
  • documentation
Jochen Weber added 2 commits Dec 5, 2019
@jochen42 jochen42 force-pushed the jochen42:redis-cache-backend branch from 3516be7 to b420894 Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.