deniskuzZ commented on code in PR #6584:
URL: https://github.com/apache/hive/pull/6584#discussion_r3579150781


##########
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:
   added



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

Reply via email to