abstractdog commented on code in PR #6584:
URL: https://github.com/apache/hive/pull/6584#discussion_r3570796939
##########
llap-server/src/test/org/apache/hadoop/hive/llap/cache/TestMetadataCache.java:
##########
@@ -185,6 +185,104 @@ public void testBuffers() throws Exception {
assertFalse(b0.incRef() > 0); // Should have also been thrown out.
}
+ /**
+ * A footer of exactly maxAlloc: the split condition used a strict {@code
<}, so no buffer was
+ * allocated and the InputStream put path returned an empty wrapper (NPE on
lock).
+ */
+ @Test
+ public void testStreamFooterEqualToMaxAlloc() throws Exception {
+ verifyFooterFromStream(newMetadataCache(), MAX_ALLOC, true);
+ }
+
+ /**
+ * A footer larger than maxAlloc is split into a multi-buffer wrapper: an
exact multiple of maxAlloc
+ * (only full chunks) and a multiple plus a remainder (full chunks + a
smaller last chunk). The
+ * remainder buffer used to be allocated with the full footer length,
exceeding the max allocation.
+ */
+ @Test
+ public void testStreamFooterLargerThanMaxAlloc() throws Exception {
+ MetadataCache cache = newMetadataCache();
+ verifyFooterFromStream(cache, 2 * MAX_ALLOC, false);
+ verifyFooterFromStream(cache, 2 * MAX_ALLOC + 3, false);
+ }
+
+ /**
+ * If reading the footer stream fails after some buffers were already
allocated, every allocated
+ * buffer must be released back to the allocator. Otherwise the long-lived
LLAP daemon leaks arena
+ * memory on every failed footer read (a truncated file, an IO error, or an
oversized chunk).
+ */
+ @Test
+ public void testStreamFooterReadFailureReleasesBuffers() {
+ assertFooterReadFailureReleasesBuffers(MAX_ALLOC); //
single-buffer path
+ assertFooterReadFailureReleasesBuffers(2 * MAX_ALLOC); //
multi-buffer, exact multiple
+ assertFooterReadFailureReleasesBuffers(2 * MAX_ALLOC + 3); //
multi-buffer, with remainder
+ }
+
+ private void assertFooterReadFailureReleasesBuffers(int length) {
+ final int arenaSize = 4096;
+ DummyMemoryManager mm = new DummyMemoryManager();
+ LlapDaemonCacheMetrics metrics = LlapDaemonCacheMetrics.create("", "");
+ BuddyAllocator alloc = new BuddyAllocator(
+ false, false, 8, MAX_ALLOC, 1, arenaSize, 0, null, mm, metrics, null,
true);
+ MetadataCache cache = new MetadataCache(alloc, mm, new DummyCachePolicy(),
true, metrics);
+
+ Object fileKey = new Object();
+ // One byte short of the footer, so reading the last chunk hits EOF
part-way through.
+ ByteArrayInputStream truncated = new ByteArrayInputStream(new byte[length
- 1]);
+ try {
+ cache.putFileMetadata(fileKey, length, truncated, null, null);
+ fail("Expected an EOFException for a truncated footer stream of length "
+ length);
+ } catch (IOException expected) {
+ // expected
Review Comment:
assert the exact EOF here that we're expecting
##########
llap-server/src/java/org/apache/hadoop/hive/llap/io/metadata/MetadataCache.java:
##########
@@ -257,41 +257,85 @@ public LlapBufferOrBuffers putFileMetadata(Object
fileKey, int length, InputStre
}
- @SuppressWarnings({ "rawtypes", "unchecked" })
+ @SuppressWarnings("unchecked")
private LlapBufferOrBuffers wrapBbForFile(LlapBufferOrBuffers result,
Object fileKey, int length, InputStream stream, CacheTag tag,
AtomicBoolean isStopped) throws IOException {
- if (result != null) return result;
+ if (result != null) {
+ return result;
+ }
int maxAlloc = allocator.getMaxAllocation();
- LlapMetadataBuffer<Object>[] largeBuffers = null;
- if (maxAlloc < length) {
- largeBuffers = new LlapMetadataBuffer[length / maxAlloc];
- for (int i = 0; i < largeBuffers.length; ++i) {
- largeBuffers[i] = new LlapMetadataBuffer<>(fileKey, tag);
+ // Buffers are locked and handed to the cache only after this returns, so
release whatever we
+ // allocated if a later read or allocation throws - otherwise it leaks
(nothing else reclaims it).
+ if (length <= maxAlloc) {
+ // The whole footer fits in a single buffer - the overwhelmingly common
case.
+ LlapMetadataBuffer<Object> buffer = new LlapMetadataBuffer<>(fileKey,
tag);
+ allocator.allocateMultiple(new MemoryBuffer[] { buffer }, length, null,
isStopped);
+ boolean done = false;
+ try {
+ readIntoCacheBuffer(stream, length, buffer);
+ done = true;
+ return buffer;
+ } finally {
+ if (!done) {
+ deallocateQuietly(buffer);
+ }
}
- allocator.allocateMultiple(largeBuffers, maxAlloc, null, isStopped);
+ }
+ // Larger footers are split across maxAlloc-sized chunks, the last one
holding the remainder.
+ LlapMetadataBuffer<Object>[] largeBuffers = new LlapMetadataBuffer[length
/ maxAlloc];
+ for (int i = 0; i < largeBuffers.length; ++i) {
+ largeBuffers[i] = new LlapMetadataBuffer<>(fileKey, tag);
+ }
+ // allocateMultiple is all-or-nothing: on success every chunk is
allocated; on failure it
+ // releases whatever it reserved, so a throw here needs no cleanup from us.
+ allocator.allocateMultiple(largeBuffers, maxAlloc, null, isStopped);
+ LlapMetadataBuffer<Object> smallBuffer = null;
Review Comment:
`remainderBuffer` to clarify what is "small" for
##########
llap-server/src/java/org/apache/hadoop/hive/llap/io/metadata/MetadataCache.java:
##########
@@ -257,41 +257,85 @@ public LlapBufferOrBuffers putFileMetadata(Object
fileKey, int length, InputStre
}
- @SuppressWarnings({ "rawtypes", "unchecked" })
+ @SuppressWarnings("unchecked")
private LlapBufferOrBuffers wrapBbForFile(LlapBufferOrBuffers result,
Object fileKey, int length, InputStream stream, CacheTag tag,
AtomicBoolean isStopped) throws IOException {
- if (result != null) return result;
+ if (result != null) {
+ return result;
+ }
int maxAlloc = allocator.getMaxAllocation();
- LlapMetadataBuffer<Object>[] largeBuffers = null;
- if (maxAlloc < length) {
- largeBuffers = new LlapMetadataBuffer[length / maxAlloc];
- for (int i = 0; i < largeBuffers.length; ++i) {
- largeBuffers[i] = new LlapMetadataBuffer<>(fileKey, tag);
+ // Buffers are locked and handed to the cache only after this returns, so
release whatever we
+ // allocated if a later read or allocation throws - otherwise it leaks
(nothing else reclaims it).
+ if (length <= maxAlloc) {
+ // The whole footer fits in a single buffer - the overwhelmingly common
case.
+ LlapMetadataBuffer<Object> buffer = new LlapMetadataBuffer<>(fileKey,
tag);
+ allocator.allocateMultiple(new MemoryBuffer[] { buffer }, length, null,
isStopped);
+ boolean done = false;
+ try {
+ readIntoCacheBuffer(stream, length, buffer);
+ done = true;
+ return buffer;
+ } finally {
+ if (!done) {
+ deallocateQuietly(buffer);
+ }
}
- allocator.allocateMultiple(largeBuffers, maxAlloc, null, isStopped);
+ }
+ // Larger footers are split across maxAlloc-sized chunks, the last one
holding the remainder.
+ LlapMetadataBuffer<Object>[] largeBuffers = new LlapMetadataBuffer[length
/ maxAlloc];
+ for (int i = 0; i < largeBuffers.length; ++i) {
+ largeBuffers[i] = new LlapMetadataBuffer<>(fileKey, tag);
+ }
+ // allocateMultiple is all-or-nothing: on success every chunk is
allocated; on failure it
+ // releases whatever it reserved, so a throw here needs no cleanup from us.
+ allocator.allocateMultiple(largeBuffers, maxAlloc, null, isStopped);
Review Comment:
nit, maybe add a line break here: next line (`smallBuffer = null`) is about
to handle a different thing
##########
llap-server/src/java/org/apache/hadoop/hive/llap/io/metadata/MetadataCache.java:
##########
@@ -257,41 +257,85 @@ public LlapBufferOrBuffers putFileMetadata(Object
fileKey, int length, InputStre
}
- @SuppressWarnings({ "rawtypes", "unchecked" })
+ @SuppressWarnings("unchecked")
private LlapBufferOrBuffers wrapBbForFile(LlapBufferOrBuffers result,
Object fileKey, int length, InputStream stream, CacheTag tag,
AtomicBoolean isStopped) throws IOException {
- if (result != null) return result;
+ if (result != null) {
+ return result;
+ }
int maxAlloc = allocator.getMaxAllocation();
- LlapMetadataBuffer<Object>[] largeBuffers = null;
- if (maxAlloc < length) {
- largeBuffers = new LlapMetadataBuffer[length / maxAlloc];
- for (int i = 0; i < largeBuffers.length; ++i) {
- largeBuffers[i] = new LlapMetadataBuffer<>(fileKey, tag);
+ // Buffers are locked and handed to the cache only after this returns, so
release whatever we
+ // allocated if a later read or allocation throws - otherwise it leaks
(nothing else reclaims it).
+ if (length <= maxAlloc) {
+ // The whole footer fits in a single buffer - the overwhelmingly common
case.
+ LlapMetadataBuffer<Object> buffer = new LlapMetadataBuffer<>(fileKey,
tag);
+ allocator.allocateMultiple(new MemoryBuffer[] { buffer }, length, null,
isStopped);
+ boolean done = false;
+ try {
+ readIntoCacheBuffer(stream, length, buffer);
+ done = true;
+ return buffer;
+ } finally {
+ if (!done) {
+ deallocateQuietly(buffer);
+ }
}
- allocator.allocateMultiple(largeBuffers, maxAlloc, null, isStopped);
+ }
+ // Larger footers are split across maxAlloc-sized chunks, the last one
holding the remainder.
+ LlapMetadataBuffer<Object>[] largeBuffers = new LlapMetadataBuffer[length
/ maxAlloc];
+ for (int i = 0; i < largeBuffers.length; ++i) {
+ largeBuffers[i] = new LlapMetadataBuffer<>(fileKey, tag);
+ }
+ // allocateMultiple is all-or-nothing: on success every chunk is
allocated; on failure it
+ // releases whatever it reserved, so a throw here needs no cleanup from us.
+ allocator.allocateMultiple(largeBuffers, maxAlloc, null, isStopped);
+ LlapMetadataBuffer<Object> smallBuffer = null;
+ boolean done = false;
+ try {
for (int i = 0; i < largeBuffers.length; ++i) {
readIntoCacheBuffer(stream, maxAlloc, largeBuffers[i]);
}
- }
- int smallSize = length % maxAlloc;
- if (smallSize == 0) {
- return new LlapMetadataBuffers(largeBuffers);
- } else {
- LlapMetadataBuffer<Object>[] smallBuffer = new LlapMetadataBuffer[1];
- smallBuffer[0] = new LlapMetadataBuffer(fileKey, tag);
- allocator.allocateMultiple(smallBuffer, length, null, isStopped);
- readIntoCacheBuffer(stream, smallSize, smallBuffer[0]);
- if (largeBuffers == null) {
- return smallBuffer[0]; // This is the overwhelmingly common case.
- } else {
- LlapMetadataBuffer<Object>[] cacheData = new
LlapMetadataBuffer[largeBuffers.length + 1];
- System.arraycopy(largeBuffers, 0, cacheData, 0, largeBuffers.length);
- cacheData[largeBuffers.length] = smallBuffer[0];
- return new LlapMetadataBuffers<>(cacheData);
+ int smallSize = length % maxAlloc;
Review Comment:
I believe this is rather "remainderSize" for clarity
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
