-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Initial support for client-side caching #3658
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
package redis.clients.jedis; | ||
|
||
import java.nio.ByteBuffer; | ||
import java.util.HashMap; | ||
import java.util.List; | ||
import java.util.Map; | ||
|
||
import redis.clients.jedis.exceptions.JedisException; | ||
import redis.clients.jedis.util.SafeEncoder; | ||
|
||
public class ClientSideCache { | ||
|
||
private final Map<ByteBuffer, Object> cache = new HashMap<>(); | ||
|
||
protected ClientSideCache() { | ||
} | ||
|
||
protected void invalidateKeys(List list) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Design discussion: As I understood Invalidation is a more complex process built around cache that involves business logic and belong to another layer, something like this:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have used Redis terms for easier recognition. Things could be changed later but not now. |
||
if (list == null) { | ||
cache.clear(); | ||
return; | ||
} | ||
|
||
list.forEach(this::invalidateKey); | ||
} | ||
|
||
private void invalidateKey(Object key) { | ||
if (key instanceof byte[]) { | ||
cache.remove(convertKey((byte[]) key)); | ||
} else { | ||
throw new JedisException("" + key.getClass().getSimpleName() + " is not supported. Value: " + String.valueOf(key)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why in this case function signature doesn't looks like |
||
} | ||
} | ||
|
||
protected void setKey(Object key, Object value) { | ||
cache.put(getMapKey(key), value); | ||
} | ||
|
||
protected <T> T getValue(Object key) { | ||
return (T) getMapValue(key); | ||
} | ||
|
||
private Object getMapValue(Object key) { | ||
return cache.get(getMapKey(key)); | ||
} | ||
|
||
private ByteBuffer getMapKey(Object key) { | ||
if (key instanceof byte[]) { | ||
return convertKey((byte[]) key); | ||
} else { | ||
return convertKey(SafeEncoder.encode(String.valueOf(key))); | ||
} | ||
} | ||
|
||
private static ByteBuffer convertKey(byte[] b) { | ||
return ByteBuffer.wrap(b); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
package redis.clients.jedis; | ||
|
||
import redis.clients.jedis.exceptions.JedisException; | ||
|
||
public class JedisClientSideCache extends Jedis { | ||
|
||
private final ClientSideCache cache; | ||
|
||
public JedisClientSideCache(final HostAndPort hostPort, final JedisClientConfig config) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about to use Proxy pattern here? Instead of introducing new connection type you can introduce a proxy object that decides whether you need to read against cache or delegate it to another "real" connection against Redis. Aggregation is always preferable against inheritance :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've found it easier to start. |
||
this(hostPort, config, new ClientSideCache()); | ||
} | ||
|
||
public JedisClientSideCache(final HostAndPort hostPort, final JedisClientConfig config, | ||
ClientSideCache cache) { | ||
super(hostPort, config); | ||
if (config.getRedisProtocol() != RedisProtocol.RESP3) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vladvildanov @dvora-h IMHO we should do this in other clients if not already there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
throw new JedisException("Client side caching is only supported with RESP3."); | ||
} | ||
|
||
this.cache = cache; | ||
this.connection.setClientSideCache(cache); | ||
clientTrackingOn(); | ||
} | ||
|
||
private void clientTrackingOn() { | ||
String reply = connection.executeCommand(new CommandObject<>( | ||
new CommandArguments(Protocol.Command.CLIENT).add("TRACKING").add("ON").add("BCAST"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct. @sazzad16 |
||
BuilderFactory.STRING)); | ||
if (!"OK".equals(reply)) { | ||
throw new JedisException("Could not enable client tracking. Reply: " + reply); | ||
} | ||
} | ||
|
||
@Override | ||
public String get(String key) { | ||
connection.readPushesWithCheckingBroken(); | ||
String cachedValue = cache.getValue(key); | ||
if (cachedValue != null) return cachedValue; | ||
|
||
String value = super.get(key); | ||
if (value != null) cache.setKey(key, value); | ||
return value; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,11 @@ public RedisInputStream(InputStream in) { | |
this(in, INPUT_BUFFER_SIZE); | ||
} | ||
|
||
public Byte peekByte() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we start adding javadocs here |
||
ensureFillSafe(); | ||
return buf[count]; | ||
} | ||
|
||
public byte readByte() throws JedisConnectionException { | ||
ensureFill(); | ||
return buf[count++]; | ||
|
@@ -252,4 +257,18 @@ private void ensureFill() throws JedisConnectionException { | |
} | ||
} | ||
} | ||
|
||
private void ensureFillSafe() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and here |
||
if (count >= limit) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. possibly silly but |
||
try { | ||
limit = in.read(buf); | ||
count = 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. count doesn't need to be within the try, can be outside |
||
if (limit == -1) { | ||
throw new JedisConnectionException("Unexpected end of stream."); | ||
} | ||
} catch (IOException e) { | ||
// do nothing | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
package redis.clients.jedis; | ||
|
||
import static org.junit.Assert.assertEquals; | ||
import static org.junit.Assert.assertNull; | ||
|
||
import org.junit.After; | ||
import org.junit.Before; | ||
import org.junit.Test; | ||
import org.mockito.InOrder; | ||
import org.mockito.Mockito; | ||
|
||
public class JedisClientSideCacheTest { | ||
|
||
protected static final HostAndPort hnp = HostAndPorts.getRedisServers().get(1); | ||
|
||
protected Jedis jedis; | ||
|
||
@Before | ||
public void setUp() throws Exception { | ||
jedis = new Jedis(hnp, DefaultJedisClientConfig.builder().timeoutMillis(500).password("foobared").build()); | ||
sazzad16 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
jedis.flushAll(); | ||
} | ||
|
||
@After | ||
public void tearDown() throws Exception { | ||
jedis.close(); | ||
} | ||
|
||
private static final JedisClientConfig configForCache = DefaultJedisClientConfig.builder() | ||
.resp3().socketTimeoutMillis(20).password("foobared").build(); | ||
|
||
@Test | ||
public void simple() { | ||
try (JedisClientSideCache jCache = new JedisClientSideCache(hnp, configForCache)) { | ||
jedis.set("foo", "bar"); | ||
assertEquals("bar", jCache.get("foo")); | ||
jedis.del("foo"); | ||
assertNull(jCache.get("foo")); | ||
} | ||
} | ||
|
||
@Test | ||
public void simpleMock() { | ||
ClientSideCache cache = Mockito.mock(ClientSideCache.class); | ||
try (JedisClientSideCache jCache = new JedisClientSideCache(hnp, configForCache, cache)) { | ||
jedis.set("foo", "bar"); | ||
assertEquals("bar", jCache.get("foo")); | ||
jedis.del("foo"); | ||
assertNull(jCache.get("foo")); | ||
} | ||
|
||
InOrder inOrder = Mockito.inOrder(cache); | ||
inOrder.verify(cache).invalidateKeys(Mockito.notNull()); | ||
inOrder.verify(cache).getValue("foo"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: store the key name in a variable and reuse |
||
inOrder.verify(cache).setKey("foo", "bar"); | ||
inOrder.verify(cache).invalidateKeys(Mockito.notNull()); | ||
inOrder.verify(cache).getValue("foo"); | ||
inOrder.verifyNoMoreInteractions(); | ||
} | ||
|
||
@Test | ||
public void flushall() { | ||
try (JedisClientSideCache jCache = new JedisClientSideCache(hnp, configForCache)) { | ||
jedis.set("foo", "bar"); | ||
assertEquals("bar", jCache.get("foo")); | ||
jedis.flushAll(); | ||
assertNull(jCache.get("foo")); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need to implement your own cache storage? Don't you have something ready to use, maybe some lightweight library that provides a cache storage API?
I'm pretty much sure it will be quite time consuming to implement search mechanism in cache to search for all the data associated with key, as well as provide metadata for telemetry (cache hits, cache misses). Also, you have to think about TTL mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should define our cache APIs. I don't think we'll find anything that perfectly matches all our use cases. With a defined set of APIs, users may choose to provide their own implementation based on their preferred library.
At some point, this will be based on a library. But we have started simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internal separation of concern is a good idea