showuon commented on code in PR #18004:
URL: https://github.com/apache/kafka/pull/18004#discussion_r1958981427
##########
core/src/test/scala/unit/kafka/log/remote/RemoteIndexCacheTest.scala:
##########
@@ -620,27 +638,29 @@ class RemoteIndexCacheTest {
val entry0 = cache.getIndexEntry(metadataList(0))
val entry1 = cache.getIndexEntry(metadataList(1))
- cache.getIndexEntry(metadataList(2))
+ val entry2 = cache.getIndexEntry(metadataList(2))
+ val entries = List(entry0, entry1, entry2)
assertCacheSize(2)
- verifyEntryIsEvicted(metadataList(0), entry0)
+ verifyEntryIsEvicted(metadataList, entries, 1)
// Reduce cache capacity to only store 1 entry
cache.resizeCacheSize(1 * estimateEntryBytesSize)
assertCacheSize(1)
- verifyEntryIsEvicted(metadataList(1), entry1)
+ verifyEntryIsEvicted(metadataList, entries, 2)
Review Comment:
The `numOfDeleted` here is confusing because we should only evict 1 entry
from the existing ones. Could we improve on it? Maybe returning the evicted
entries from previous `verifyEntryIsEvicted`, or `getEntriedsKept`, or adding
some more comments to make it clear?
##########
core/src/test/scala/unit/kafka/log/remote/RemoteIndexCacheTest.scala:
##########
@@ -957,6 +977,16 @@ class RemoteIndexCacheTest {
}
}
+ private def verifyFetchIndexInvocationWithRange(lower: Int,
+ upper: Int,
+ indexTypes: Seq[IndexType] =
+ Seq(IndexType.OFFSET,
IndexType.TIMESTAMP, IndexType.TRANSACTION)): Unit = {
+ for (indexType <- indexTypes) {
+ verify(rsm,
atLeast(lower)).fetchIndex(any(classOf[RemoteLogSegmentMetadata]),
ArgumentMatchers.eq(indexType))
+ verify(rsm,
atMost(upper)).fetchIndex(any(classOf[RemoteLogSegmentMetadata]),
ArgumentMatchers.eq(indexType))
+ }
Review Comment:
I love the idea of verifying lower/upper bound.
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
