cadonna commented on a change in pull request #9779:
URL: https://github.com/apache/kafka/pull/9779#discussion_r596741592
##########
File path:
streams/src/test/java/org/apache/kafka/streams/state/internals/ThreadCacheTest.java
##########
@@ -252,20 +262,43 @@ public void shouldGetSameKeyAsPeekNext() {
assertEquals(iterator.peekNextKey(), iterator.next().key);
}
+ @Test
+ public void shouldGetSameKeyAsPeekNextReverseRange() {
+ final ThreadCache cache = new ThreadCache(logContext, 10000L, new
MockStreamsMetrics(new Metrics()));
+ final Bytes theByte = Bytes.wrap(new byte[]{1});
+ cache.put(namespace, theByte, dirtyEntry(theByte.get()));
+ final ThreadCache.MemoryLRUCacheBytesIterator iterator =
cache.reverseRange(namespace, Bytes.wrap(new byte[]{0}), theByte);
+ assertEquals(iterator.peekNextKey(), iterator.next().key);
Review comment:
nit: see my comment about `assertThat()` above.
##########
File path:
streams/src/test/java/org/apache/kafka/streams/state/internals/ThreadCacheTest.java
##########
@@ -252,20 +262,43 @@ public void shouldGetSameKeyAsPeekNext() {
assertEquals(iterator.peekNextKey(), iterator.next().key);
}
+ @Test
+ public void shouldGetSameKeyAsPeekNextReverseRange() {
+ final ThreadCache cache = new ThreadCache(logContext, 10000L, new
MockStreamsMetrics(new Metrics()));
+ final Bytes theByte = Bytes.wrap(new byte[]{1});
+ cache.put(namespace, theByte, dirtyEntry(theByte.get()));
+ final ThreadCache.MemoryLRUCacheBytesIterator iterator =
cache.reverseRange(namespace, Bytes.wrap(new byte[]{0}), theByte);
+ assertEquals(iterator.peekNextKey(), iterator.next().key);
+ }
+
@Test
public void shouldThrowIfNoPeekNextKey() {
final ThreadCache cache = new ThreadCache(logContext, 10000L, new
MockStreamsMetrics(new Metrics()));
final ThreadCache.MemoryLRUCacheBytesIterator iterator =
cache.range(namespace, Bytes.wrap(new byte[]{0}), Bytes.wrap(new byte[]{1}));
assertThrows(NoSuchElementException.class, iterator::peekNextKey);
}
+ @Test
+ public void shouldThrowIfNoPeekNextKeyReverseRange() {
+ final ThreadCache cache = new ThreadCache(logContext, 10000L, new
MockStreamsMetrics(new Metrics()));
+ final ThreadCache.MemoryLRUCacheBytesIterator iterator =
cache.reverseRange(namespace, Bytes.wrap(new byte[]{0}), Bytes.wrap(new
byte[]{1}));
+ assertThrows(NoSuchElementException.class, iterator::peekNextKey);
+ }
+
Review comment:
nit: could you try to deduplicate code here and in the other unit tests?
Here for example, you could have one method like this:
```
private void shouldThrowIfNoPeekNextKey(final
Supplier<MemoryLRUCacheBytesIterator> methodUnderTest) {
final ThreadCache.MemoryLRUCacheBytesIterator iterator =
methodUnderTest.get();
assertThrows(NoSuchElementException.class, iterator::peekNextKey);
}
```
and then two public tests
```
@Test
public void shouldThrowIfNoPeekNextKeyRange() {
final ThreadCache cache = new ThreadCache(logContext, 10000L, new
MockStreamsMetrics(new Metrics()));
shouldThrowIfNoPeekNextKey(() -> cache.range(namespace,
Bytes.wrap(new byte[]{0}), Bytes.wrap(new byte[]{1})));
}
@Test
public void shouldThrowIfNoPeekNextKeyReverseRange() {
final ThreadCache cache = new ThreadCache(logContext, 10000L, new
MockStreamsMetrics(new Metrics()));
shouldThrowIfNoPeekNextKey(() -> cache.reverseRange(namespace,
Bytes.wrap(new byte[]{0}), Bytes.wrap(new byte[]{1})));
}
```
Admittedly, in this specific case, we would not win much but for other unit
tests in this test class it may be worth. Try and then decide if it is worth or
not.
##########
File path:
streams/src/test/java/org/apache/kafka/streams/state/internals/ThreadCacheTest.java
##########
@@ -243,6 +243,16 @@ public void shouldPeekNextKey() {
assertEquals(theByte, iterator.peekNextKey());
}
+ @Test
+ public void shouldPeekNextKeyReverseRange() {
+ final ThreadCache cache = new ThreadCache(logContext, 10000L, new
MockStreamsMetrics(new Metrics()));
+ final Bytes theByte = Bytes.wrap(new byte[]{1});
+ cache.put(namespace, theByte, dirtyEntry(theByte.get()));
+ final ThreadCache.MemoryLRUCacheBytesIterator iterator =
cache.reverseRange(namespace, Bytes.wrap(new byte[]{0}), theByte);
+ assertEquals(theByte, iterator.peekNextKey());
+ assertEquals(theByte, iterator.peekNextKey());
Review comment:
nit: Could you please use `assertThat(iterator.peekNextKey(), is(the
Byte))` here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
