showuon commented on code in PR #18004:
URL: https://github.com/apache/kafka/pull/18004#discussion_r1959141446
##########
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:
Sorry that I still think it's not clear. Here how I read the test:
1. Increase cache capacity to only store 2 entries
2. Insert 3 entries into the cache and verify it only contains 2 entries,
and 1 is evicted.
3. Resize the cache size to only store 1 entry
4. So, the `existing 2 entries in cache` should have 1 entry got evicted
after resize, not 2.
For (4), we should explain why after resize from 2 -> 1, there are 2 entries
marked as deleted, not 1. That's why I think we can also do something like this:
```
// Increase cache capacity to only store 2 entries
cache.resizeCacheSize(2 * estimateEntryBytesSize)
assertCacheSize(0)
val entry0 = cache.getIndexEntry(metadataList(0))
val entry1 = cache.getIndexEntry(metadataList(1))
val entry2 = cache.getIndexEntry(metadataList(2))
val entries = List(entry0, entry1, entry2)
assertCacheSize(2)
// return entries evicted
val evictedEntries = verifyEntryIsEvicted(metadataList, entries, 1)
// Reduce cache capacity to only store 1 entry
cache.resizeCacheSize(1 * estimateEntryBytesSize)
//
val entriesInCache = entries.filterNot(entry =>
evictedEntries.contain(entry))
assertCacheSize(1)
// use `entriesInCache` here, so that we can have 1 `numMarkedAsDeleted`
verifyEntryIsEvicted(metadataList, entriesInCache, 1)
```
Of course updating the comment is OK, just need to explain it clearly.
--
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
