You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/12/22 13:15:53 UTC
[GitHub] [kafka] vamossagar12 opened a new pull request #9779: KAFKA-10767: Adding test cases for all, reverseAll and reverseRange for ThreadCache
vamossagar12 opened a new pull request #9779:
URL: https://github.com/apache/kafka/pull/9779
The test cases for ThreaCache didn't have the corresponding unit tests for all, reverseAll and reverseRange methods. This PR aims to add the same.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #9779: KAFKA-10767: Adding test cases for all, reverseAll and reverseRange for ThreadCache
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on pull request #9779:
URL: https://github.com/apache/kafka/pull/9779#issuecomment-756273125
@cadonna , could you plz review this PR whenever you get the chance? Thanks!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] cadonna merged pull request #9779: KAFKA-10767: Adding test cases for all, reverseAll and reverseRange for ThreadCache
Posted by GitBox <gi...@apache.org>.
cadonna merged pull request #9779:
URL: https://github.com/apache/kafka/pull/9779
--
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:
users@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #9779: KAFKA-10767: Adding test cases for all, reverseAll and reverseRange for ThreadCache
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on pull request #9779:
URL: https://github.com/apache/kafka/pull/9779#issuecomment-801596097
@cadonna no worries :)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on a change in pull request #9779: KAFKA-10767: Adding test cases for all, reverseAll and reverseRange for ThreadCache
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on a change in pull request #9779:
URL: https://github.com/apache/kafka/pull/9779#discussion_r597617935
##########
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:
done
--
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:
users@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #9779: KAFKA-10767: Adding test cases for all, reverseAll and reverseRange for ThreadCache
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on pull request #9779:
URL: https://github.com/apache/kafka/pull/9779#issuecomment-791122717
@cadonna , could you plz review this PR as well whenevr you get the chance?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #9779: KAFKA-10767: Adding test cases for all, reverseAll and reverseRange for ThreadCache
Posted by GitBox <gi...@apache.org>.
cadonna commented on pull request #9779:
URL: https://github.com/apache/kafka/pull/9779#issuecomment-801410030
@vamossagar12 I am sorry that I haven't reviewed you PR yet. I will try to review it as soon as possible which can still take some time currently.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on a change in pull request #9779: KAFKA-10767: Adding test cases for all, reverseAll and reverseRange for ThreadCache
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #9779: KAFKA-10767: Adding test cases for all, reverseAll and reverseRange for ThreadCache
Posted by GitBox <gi...@apache.org>.
cadonna commented on pull request #9779:
URL: https://github.com/apache/kafka/pull/9779#issuecomment-832581620
Unrelated test failure:
Build / JDK 8 and Scala 2.12 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopics()
--
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:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #9779: KAFKA-10767: Adding test cases for all, reverseAll and reverseRange for ThreadCache
Posted by GitBox <gi...@apache.org>.
cadonna commented on pull request #9779:
URL: https://github.com/apache/kafka/pull/9779#issuecomment-832582595
Merged! Thank you @vamossagar12 and sorry for the late merge.
--
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:
users@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #9779: KAFKA-10767: Adding test cases for all, reverseAll and reverseRange for ThreadCache
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on pull request #9779:
URL: https://github.com/apache/kafka/pull/9779#issuecomment-822621642
hey @guozhangwang , would you be able to review this whenever you get the chance?
--
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:
users@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on a change in pull request #9779: KAFKA-10767: Adding test cases for all, reverseAll and reverseRange for ThreadCache
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on a change in pull request #9779:
URL: https://github.com/apache/kafka/pull/9779#discussion_r597618566
##########
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:
Done. This made sense. I have also added the logic to generate the ThreadCache in a separate method(`setupThreadCache`) . That reduced lot of duplicate code from a lot of methods
--
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:
users@infra.apache.org