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 <40628386+sz...@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(),