[hive] branch master updated: HIVE-24297: LLAP buffer collision causes NPE (Adam Szita, reviewed by Antal Sinkovits and Marta Kuczora)

[sz...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

szita pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/hive.git


The following commit(s) were added to refs/heads/master by this push:
     new 3754335  HIVE-24297: LLAP buffer collision causes NPE (Adam Szita, reviewed by Antal Sinkovits and Marta Kuczora)
3754335 is described below

commit 375433510b73c5a22bde4e13485dfc16eaa24706
Author: Adam Szita <40...@users.noreply.github.com>
AuthorDate: Wed Oct 28 15:43:49 2020 +0100

    HIVE-24297: LLAP buffer collision causes NPE (Adam Szita, reviewed by Antal Sinkovits and Marta Kuczora)
---
 .../hadoop/hive/llap/cache/LowLevelCacheImpl.java  |  4 +++
 .../hive/llap/cache/TestLowLevelCacheImpl.java     | 38 ++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelCacheImpl.java b/llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelCacheImpl.java
index 4b77361..59fedbd 100644
--- a/llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelCacheImpl.java
+++ b/llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelCacheImpl.java
@@ -338,6 +338,10 @@ public class LowLevelCacheImpl implements LowLevelCache, BufferUsageManager, Lla
                   buffer, oldVal);
             }
 
+            // This is always set to ranges[i].getLength() prior inserting into the map to avoid inconsistency with the
+            // check above. However once we decided that this new buffer will not be cached, we should unset
+            // declaredCachedLength, so that it can be instantly deallocated at unlockBuffer()'s else branch.
+            buffer.declaredCachedLength = LlapDataBuffer.UNKNOWN_CACHED_LENGTH;
             unlockBuffer(buffer, false);
             buffers[i] = oldVal;
             if (result == null) {
diff --git a/llap-server/src/test/org/apache/hadoop/hive/llap/cache/TestLowLevelCacheImpl.java b/llap-server/src/test/org/apache/hadoop/hive/llap/cache/TestLowLevelCacheImpl.java
index 77c379c..11a0128 100644
--- a/llap-server/src/test/org/apache/hadoop/hive/llap/cache/TestLowLevelCacheImpl.java
+++ b/llap-server/src/test/org/apache/hadoop/hive/llap/cache/TestLowLevelCacheImpl.java
@@ -230,6 +230,44 @@ Example code to test specific scenarios:
   }
 
   @Test
+  public void testDeclaredLengthUnsetForCollidedBuffer() throws Exception {
+    LowLevelCacheImpl cache = new LowLevelCacheImpl(
+        LlapDaemonCacheMetrics.create("test", "1"), new DummyCachePolicy(),
+        new DummyAllocator(), true, -1); // no cleanup thread
+
+    long fileKey = 1;
+    long[] putResult;
+
+    // 5 buffers: 0,1 cached in the first go, 2,3,4 cached in the next one; buffers 1 and 4 cover overlapping ranges
+    LlapDataBuffer[] buffers = IntStream.range(0, 5).mapToObj(i -> fb()).toArray(LlapDataBuffer[]::new);
+
+    LlapDataBuffer[] oldBufs = new LlapDataBuffer[]{ buffers[0], buffers[1] };
+    DiskRange[] oldRanges = drs(0, 15);
+
+    LlapDataBuffer[] newBufs = new LlapDataBuffer[]{ buffers[2], buffers[3], buffers[4] };
+    DiskRange[] newRanges = drs(5, 10, 15);
+
+    putResult = cache.putFileData(fileKey, oldRanges, oldBufs, 0, Priority.NORMAL, null, null);
+    assertNull(putResult);
+
+    putResult = cache.putFileData(fileKey, newRanges, newBufs, 0, Priority.NORMAL, null, null);
+    assertEquals(1, putResult.length);
+    assertEquals(Long.parseLong("100", 2), putResult[0]);
+
+    for (int i = 0; i < buffers.length; ++i) {
+      // since buffer 4 collided with buffer 1 it should not have cached length set, as it will not even be seen by
+      // cache policy before it gets quickly deallocated in processCollisions method
+      if (i == buffers.length - 1) {
+        assertEquals(LlapDataBuffer.UNKNOWN_CACHED_LENGTH, buffers[i].declaredCachedLength);
+      } else {
+        assertEquals(1, buffers[i].declaredCachedLength);
+      }
+    }
+
+
+  }
+
+  @Test
   public void testMultiMatch() {
     LowLevelCacheImpl cache = new LowLevelCacheImpl(
         LlapDaemonCacheMetrics.create("test", "1"), new DummyCachePolicy(),