[GitHub] [accumulo] keith-turner commented on a change in pull request #1333: LRUCache is Evicting Too Many CachedBlocks

[GitBox <gi...@apache.org>]

keith-turner commented on a change in pull request #1333: LRUCache is Evicting Too Many CachedBlocks
URL: https://github.com/apache/accumulo/pull/1333#discussion_r335658935
 
 

 ##########
 File path: core/src/test/java/org/apache/accumulo/core/file/blockfile/cache/TestCachedBlockQueue.java
 ##########
 @@ -112,17 +166,20 @@ public void testQueueSmallBlockEdgeCase() {
 
     assertEquals(queue.heapSize(), expectedSize);
 
-    LinkedList<org.apache.accumulo.core.file.blockfile.cache.lru.CachedBlock> blocks =
-        queue.getList();
-    assertEquals(blocks.poll().getName(), "cb0");
-    assertEquals(blocks.poll().getName(), "cb1");
-    assertEquals(blocks.poll().getName(), "cb2");
-    assertEquals(blocks.poll().getName(), "cb3");
-    assertEquals(blocks.poll().getName(), "cb4");
-    assertEquals(blocks.poll().getName(), "cb5");
-    assertEquals(blocks.poll().getName(), "cb6");
-    assertEquals(blocks.poll().getName(), "cb7");
-    assertEquals(blocks.poll().getName(), "cb8");
+    Iterator<org.apache.accumulo.core.file.blockfile.cache.lru.CachedBlock> blocks =
+        queue.getBlocks().iterator();
+    assertEquals(blocks.next().getName(), "cb0");
+    assertEquals(blocks.next().getName(), "cb1");
+    assertEquals(blocks.next().getName(), "cb2");
+    assertEquals(blocks.next().getName(), "cb3");
+    assertEquals(blocks.next().getName(), "cb4");
+    assertEquals(blocks.next().getName(), "cb5");
+    assertEquals(blocks.next().getName(), "cb6");
+    assertEquals(blocks.next().getName(), "cb7");
+    assertEquals(blocks.next().getName(), "cb8");
+    assertEquals(blocks.next().getName(), "cb9");
 
 Review comment:
   I made the following change locally and this test no longer works (even with removing cb9 and cb10 from the expected values).
   
   ```java
     private static class CachedBlock
         extends org.apache.accumulo.core.file.blockfile.cache.lru.CachedBlock {
       public CachedBlock(long heapSize, String name, long accessTime) {
         super(name, new byte[(int) (heapSize - CachedBlock.PER_BLOCK_OVERHEAD)], accessTime, false);
         recordSize(new AtomicLong(0));
       }
     }
   ```
   
   This test has the following comment in it :
   
   ```java
       // This is older so we must include it, but it will not end up kicking
       // anything out because (heapSize - cb8.heapSize + cb0.heapSize < maxSize)
       // and we must always maintain heapSize >= maxSize once we achieve it.
   ```
   
   I am not completely sure, but I think the changes you made may cause heapSize to drop below max size after it was achieved.   However, I am not sure what the significance of this is, need to analyze the code that uses cached block.  

----------------------------------------------------------------
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


With regards,
Apache Git Services