-
Notifications
You must be signed in to change notification settings - Fork 119
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
Client-side caching support #402
base: master
Are you sure you want to change the base?
Conversation
src/main/java/io/vertx/redis/client/impl/CachingRedisClientImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/vertx/redis/client/impl/cache/LRUClientSideCache.java
Outdated
Show resolved
Hide resolved
I don't feel qualified to review the caching code, but it seems like the 2 concurrent maps are assumed to be synchronized (not the Java language Also, not sure if this was intentional or not, but it seems kinda like a big deal: if you have a EDIT: I mean, there would still likely have to be a caching-aware client, which would maintain the cache itself and the invalidation connection. All connections created from this client would share the caching stuff. It likely doesn't have to be a new user-visible interface, though. |
@Ladicek thanks for all of the points, and I also personally had the debate about client vs. connection. I decided to start here with the biggest target and use-case being for the I do agree the most robust solution would be to directly integrate with the connection implementation to configure tracking based on options, but I wasn't sure where the invalidations listener and cache manager would live in such a case. Without going down the road of verticles or workers or using the event bus, it felt easier to make everything explicit and opt-in with a direct client. I could perhaps remove the |
@craig-day OK, that seems like a valid decision, but I guess it should be documented then? |
bc89e2c
to
fc7ce99
Compare
@vietj @Ladicek I finally got back around to this. I've pushed a number of updates and I think this is ready for review. Some notes on the design choices. I chose to stick with a wrapper for a few reasons:
I considered an alternative approach like creating a As an optimization, however, I did make the connections somewhat cache-aware. Each connection tracks whether it has had client tracking configured. This is done to avoid sending 2 commands for every command a client sends. Once a connection has had client tracking configured, it doesn't need to change unless it gets explicitly turned off. On that note, currently a client could "break things" if they were to enable caching, checkout a connection, and then send the |
@Ladicek I think the last commit I just pushed is doing most of what you wanted. I removed the client creation away from the wrapper and into the default |
simplify cache impls; some javadocs and tests more javadocs; invalidation handler on client basic support for max age in default impls initial PR feedback; naming consistencies checkpoint on wrapped connection push tracking status down to actual conn impl simplify cache impls to just LRU backed by LinkedHashMap cleanup client wrapper; test simple operation docs update make caching configured via options, not an explicit wrapper regenerate options converters
059ef61
to
ff1f481
Compare
Motivation:
Redis has some commands and server side support for clients to implement client-side caching by using a key tracking approach.
This proposes adding support by defining a new
CachingRedis
interface that extendsRedis
, along with aRedisClientCache
interface to act as the local store. The client implementation will then checkout and maintain a long-lived connection that will act in subscribe mode to listen for invalidations. This is the approach required for the RESP2 protocol, but technically isn't required for RESP3 since connections can multiplex data and response types. However, I chose to keep using this approach to keep things simple by leveraging a long-lived connection as a read stream in subscribe mode.I also included a simple implementation of a local cache store, an LRU. I'd be happy to exclude this if we want consumers to be forced to supply their own, but I started with something to make it usable out of the box.
Conformance:
Your commits should be signed and you should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md
Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines