-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-25507 Leak of ESTABLISHED sockets when compaction encountered "… #2882
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
…java.io.IOException: Invalid HFile block magic"
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -321,10 +321,10 @@ protected final List<Path> compact(final CompactionRequestImpl request, | |
dropCache = this.dropCacheMinor; | ||
} | ||
|
||
List<StoreFileScanner> scanners = | ||
createFileScanners(request.getFiles(), smallestReadPoint, dropCache); | ||
InternalScanner scanner = null; | ||
boolean finished = false; | ||
List<StoreFileScanner> scanners = | ||
createFileScanners(request.getFiles(), smallestReadPoint, dropCache); | ||
try { | ||
/* Include deletes, unless we are doing a major compaction */ | ||
ScanType scanType = scannerFactory.getScanType(request); | ||
|
@@ -345,7 +345,18 @@ protected final List<Path> compact(final CompactionRequestImpl request, | |
+ 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, @saintstack . I check the codes of KVHeap and RegionScanner, they seem OK. Thanks your suggests, I also think createFileScanners should be inside the try/finally. If we only close the scanners when InnerScanner is null, may be we can avoid to close the scanners twice. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me @sunhelly . Does that mean update to the PR? Thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi, @saintstack , the PR has been updated. |
||
} | ||
if (!finished && writer != null) { | ||
abortWriter(writer); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is nice find.