-
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
Support for client-side caching - phase 2 #3673
Changes from all commits
f2dc974
ab8b774
0d04ee3
f3a6271
265310a
24036dd
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 |
---|---|---|
|
@@ -43,9 +43,9 @@ public RedisInputStream(InputStream in) { | |
this(in, INPUT_BUFFER_SIZE); | ||
} | ||
|
||
public Byte peekByte() { | ||
ensureFillSafe(); | ||
return buf[count]; | ||
public boolean peek(byte b) throws JedisConnectionException { | ||
ensureFill(); // in current design, at least one reply is expected. so ensureFillSafe() is not necessary. | ||
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. But, what happens if there is no reply? It shouldn't occur, but maybe there's something we can do for resiliency here. At the very least raise a data specific exception so that we can know this happened? 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. @chayim A JedisConnectionException wrapping a ReadTimeoutException will be thrown. |
||
return buf[count] == b; | ||
} | ||
|
||
public byte readByte() throws JedisConnectionException { | ||
|
@@ -257,18 +257,4 @@ private void ensureFill() throws JedisConnectionException { | |
} | ||
} | ||
} | ||
|
||
private void ensureFillSafe() { | ||
if (count >= limit) { | ||
try { | ||
limit = in.read(buf); | ||
count = 0; | ||
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 |
---|---|---|
@@ -1,8 +1,10 @@ | ||
package redis.clients.jedis; | ||
|
||
import static org.hamcrest.MatcherAssert.assertThat; | ||
import static org.junit.Assert.assertEquals; | ||
import static org.junit.Assert.assertNull; | ||
|
||
import org.hamcrest.Matchers; | ||
import org.junit.After; | ||
import org.junit.Before; | ||
import org.junit.Test; | ||
|
@@ -17,7 +19,7 @@ public class JedisClientSideCacheTest { | |
|
||
@Before | ||
public void setUp() throws Exception { | ||
jedis = new Jedis(hnp, DefaultJedisClientConfig.builder().timeoutMillis(500).password("foobared").build()); | ||
jedis = new Jedis(hnp, DefaultJedisClientConfig.builder().password("foobared").build()); | ||
jedis.flushAll(); | ||
} | ||
|
||
|
@@ -26,45 +28,83 @@ public void tearDown() throws Exception { | |
jedis.close(); | ||
} | ||
|
||
private static final JedisClientConfig configForCache = DefaultJedisClientConfig.builder() | ||
.resp3().socketTimeoutMillis(20).password("foobared").build(); | ||
private static final JedisClientConfig clientConfig = DefaultJedisClientConfig.builder().resp3().password("foobared").build(); | ||
|
||
@Test | ||
public void simple() { | ||
try (JedisClientSideCache jCache = new JedisClientSideCache(hnp, configForCache)) { | ||
try (JedisClientSideCache jCache = new JedisClientSideCache(hnp, clientConfig)) { | ||
jedis.set("foo", "bar"); | ||
assertEquals("bar", jCache.get("foo")); | ||
jedis.del("foo"); | ||
assertNull(jCache.get("foo")); | ||
assertThat(jCache.get("foo"), Matchers.oneOf("bar", null)); // ? | ||
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 this be achieved without adding this import? 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. @chayim We'd have to write a "utility" method. 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. General desire to have fewer dependencies.. even if it's just for test. Not a hard requirement. |
||
} | ||
} | ||
|
||
@Test | ||
public void simpleMock() { | ||
public void simpleMoreAndMock() { | ||
ClientSideCache cache = Mockito.mock(ClientSideCache.class); | ||
try (JedisClientSideCache jCache = new JedisClientSideCache(hnp, configForCache, cache)) { | ||
Mockito.when(cache.getValue("foo")).thenReturn(null, "bar", null); | ||
|
||
try (JedisClientSideCache jCache = new JedisClientSideCache(hnp, clientConfig, cache)) { | ||
jedis.set("foo", "bar"); | ||
|
||
assertEquals("bar", jCache.get("foo")); | ||
|
||
jedis.del("foo"); | ||
|
||
assertEquals("bar", jCache.get("foo")); | ||
|
||
// there should be an invalid pending; any connection command will make it read | ||
jCache.ping(); | ||
|
||
assertNull(jCache.get("foo")); | ||
} | ||
|
||
InOrder inOrder = Mockito.inOrder(cache); | ||
inOrder.verify(cache).invalidateKeys(Mockito.notNull()); | ||
inOrder.verify(cache).getValue("foo"); | ||
inOrder.verify(cache).setKey("foo", "bar"); | ||
inOrder.verify(cache).getValue("foo"); | ||
inOrder.verify(cache).invalidateKeys(Mockito.notNull()); | ||
inOrder.verify(cache).getValue("foo"); | ||
inOrder.verifyNoMoreInteractions(); | ||
} | ||
|
||
@Test | ||
public void flushall() { | ||
try (JedisClientSideCache jCache = new JedisClientSideCache(hnp, configForCache)) { | ||
public void flushAll() { | ||
try (JedisClientSideCache jCache = new JedisClientSideCache(hnp, clientConfig)) { | ||
jedis.set("foo", "bar"); | ||
assertEquals("bar", jCache.get("foo")); | ||
jedis.flushAll(); | ||
assertThat(jCache.get("foo"), Matchers.oneOf("bar", null)); // ? | ||
} | ||
} | ||
|
||
@Test | ||
public void flushAllMoreAndMock() { | ||
ClientSideCache cache = Mockito.mock(ClientSideCache.class); | ||
Mockito.when(cache.getValue("foo")).thenReturn(null, "bar", null); | ||
|
||
try (JedisClientSideCache jCache = new JedisClientSideCache(hnp, clientConfig, cache)) { | ||
jedis.set("foo", "bar"); | ||
|
||
assertEquals("bar", jCache.get("foo")); | ||
|
||
jedis.flushAll(); | ||
|
||
assertEquals("bar", jCache.get("foo")); | ||
|
||
// there should be an invalid pending; any connection command will make it read | ||
jCache.ping(); | ||
|
||
assertNull(jCache.get("foo")); | ||
} | ||
|
||
InOrder inOrder = Mockito.inOrder(cache); | ||
inOrder.verify(cache).getValue("foo"); | ||
inOrder.verify(cache).setKey("foo", "bar"); | ||
inOrder.verify(cache).getValue("foo"); | ||
inOrder.verify(cache).invalidateKeys(Mockito.isNull()); | ||
inOrder.verify(cache).getValue("foo"); | ||
inOrder.verifyNoMoreInteractions(); | ||
} | ||
} |
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.
Correct, though make sure you're cognizant of (hopefully) future support for broadcast and friends.