[GitHub] [hbase] saintstack commented on a change in pull request #2882: HBASE-25507 Leak of ESTABLISHED sockets when compaction encountered "…

[GitBox <gi...@apache.org>]

saintstack commented on a change in pull request #2882:
URL: https://github.com/apache/hbase/pull/2882#discussion_r557863701



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/Compactor.java
##########
@@ -345,7 +345,17 @@ private InternalScanner postCompactScannerOpen(CompactionRequestImpl request, Sc
             + store.getRegionInfo().getRegionNameAsString() + " because it was interrupted.");
       }
     } finally {
-      Closeables.close(scanner, true);
+      // createScanner may fail when seeking hfiles encounter Exception, e.g. even only one hfile
+      // reader encounters java.io.IOException: Invalid HFile block magic: \x00\x00\x00\x00\x00\x00\x00\x00
+      // and then scanner will be null, but scanners for all the hfiles should be closed,
+      // or else we will find leak of ESTABLISHED sockets.
+      if (scanner == null) {
+        for (StoreFileScanner sfs : scanners) {
+          sfs.close();
+        }
+      } else {
+        Closeables.close(scanner, true);

Review comment:
       First, nice debugging. Good find.
   
   Looking at this, I wonder if other InternalScanners can fail in similar way and leak? I see the InternalScanner is implemented in a few places... in KVHeap, and RegionScanner. Could these have the same problem?
   
   Would it be more clean if the createFileScanners were inside a try/finally? Or will scanners act strangely if closed twice... once by internalscanner on the happy path, and then again in the outer finally?
   
   Otherwise, nice work. Above are nits.... just stuff to consider.
   
   




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org