SOLR-10116: add more comments, improve variable names in BlockCache

solr/core/src/java/org/apache/solr/store/blockcache/BlockCache.java

@@ -102,6 +102,17 @@ public class BlockCache {
     metrics.blockCacheSize.decrementAndGet();
   }
 
+  /**
+   * This is only best-effort... it's possible for false to be returned.
+   * The blockCacheKey is cloned before it is inserted into the map, so it may be reused by clients if desired.
+   *
+   * @param blockCacheKey the key for the block
+   * @param blockOffset the offset within the block
+   * @param data source data to write to the block
+   * @param offset offset within the source data array
+   * @param length the number of bytes to write.
+   * @return true if the block was cached/updated
+   */
   public boolean store(BlockCacheKey blockCacheKey, int blockOffset,
       byte[] data, int offset, int length) {
     if (length + blockOffset > blockSize) {
@@ -115,12 +126,19 @@ public class BlockCache {
       newLocation = true;
       location = new BlockCacheLocation();
       if (!findEmptyLocation(location)) {
+        // YCS: it looks like when the cache is full (a normal scenario), then two concurrent writes will result in one of them failing
+        // because no eviction is done first.  The code seems to rely on leaving just a single block empty.
+        // TODO: simplest fix would be to leave more than one block empty
         return false;
       }
     }
+
+    // YCS: I think this means that the block existed, but it is in the process of being
+    // concurrently removed.  This flag is set in the releaseLocation eviction listener.
     if (location.isRemoved()) {
       return false;
     }
+
     int bankId = location.getBankId();
     int bankOffset = location.getBlock() * blockSize;
     ByteBuffer bank = getBank(bankId);
@@ -133,6 +151,14 @@ public class BlockCache {
     return true;
   }
 
+  /**
+   * @param blockCacheKey the key for the block
+   * @param buffer the target buffer for the read result
+   * @param blockOffset offset within the block
+   * @param off offset within the target buffer
+   * @param length the number of bytes to read
+   * @return true if the block was cached and the bytes were read
+   */
   public boolean fetch(BlockCacheKey blockCacheKey, byte[] buffer,
       int blockOffset, int off, int length) {
     BlockCacheLocation location = cache.getIfPresent(blockCacheKey);
@@ -140,13 +166,14 @@ public class BlockCache {
       return false;
     }
     if (location.isRemoved()) {
+      // location is in the process of being removed and the block may have already been reused by this point.
       return false;
     }
     int bankId = location.getBankId();
-    int offset = location.getBlock() * blockSize;
+    int bankOffset = location.getBlock() * blockSize;
     location.touch();
     ByteBuffer bank = getBank(bankId);
-    bank.position(offset + blockOffset);
+    bank.position(bankOffset + blockOffset);
     bank.get(buffer, off, length);
     return true;
   }
@@ -201,10 +228,12 @@ public class BlockCache {
     }
   }
 
+  /** Returns a new copy of the ByteBuffer for the given bank, so it's safe to call position() on w/o additional synchronization */
   private ByteBuffer getBank(int bankId) {
     return banks[bankId].duplicate();
   }
 
+  /** returns the number of elements in the cache */
   public int getSize() {
     return cache.asMap().size();
   }

solr/core/src/java/org/apache/solr/store/blockcache/BlockCacheLocation.java

@@ -35,6 +35,7 @@ public class BlockCacheLocation {
     touch();
   }
 
+  /** The block within the bank.  This has no relationship to the blockId in BlockCacheKey */
   public void setBlock(int block) {
     this.block = block;
   }
@@ -43,6 +44,7 @@ public class BlockCacheLocation {
     this.bankId = bankId;
   }
 
+  /** The block within the bank.  This has no relationship to the blockId in BlockCacheKey */
   public int getBlock() {
     return block;
   }