HBASE-25507 Leak of ESTABLISHED sockets when compaction encountered "… #2882
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
sunhelly
force-pushed
the
HBASE-25507
branch
2 times, most recently
from
0421634 to
ef456ab
Compare
saintstack
approved these changes
Jan 15, 2021
| sfs.close(); | ||
| } | ||
| } else { | ||
| Closeables.close(scanner, true); |
There was a problem hiding this 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.
There was a problem hiding this comment.
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.
Sounds good to me @sunhelly . Does that mean update to the PR? Thanks.
There was a problem hiding this comment.
Hi, @saintstack , the PR has been updated.
Function createFileScanners closes the store file scanners it created when encounters any exception, so it is out the try/finally. When the InternalScanner which contains all the store file scanners is not null, all the scanners will be processed when it closes. So I added a check for InternalScanner if it is null, if so, close the store file scanners manually in finally.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
…java.io.IOException: Invalid HFile block magic"
|
getScannersForCompaction close scanners when it encounters exception, so let createFileScanners outside the try/finally. |
ramkrish86
reviewed
Jan 18, 2021
| // 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) { |
ramkrish86
approved these changes
Jan 18, 2021
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
saintstack
pushed a commit
that referenced
this pull request
Feb 9, 2021
…java.io.IOException: Invalid HFile block magic" (#2882) Signed-off-by: Ramkrishna <ramkrishna@apache.org> Signed-off-by: stack <stack@apache.org>
saintstack
pushed a commit
that referenced
this pull request
Feb 9, 2021
…java.io.IOException: Invalid HFile block magic" (#2882) Signed-off-by: Ramkrishna <ramkrishna@apache.org> Signed-off-by: stack <stack@apache.org>
saintstack
pushed a commit
that referenced
this pull request
Feb 9, 2021
…java.io.IOException: Invalid HFile block magic" (#2882) Signed-off-by: Ramkrishna <ramkrishna@apache.org> Signed-off-by: stack <stack@apache.org>
ddupg
pushed a commit
to ddupg/hbase
that referenced
this pull request
Feb 19, 2021
…java.io.IOException: Invalid HFile block magic" (apache#2882) Signed-off-by: Ramkrishna <ramkrishna@apache.org> Signed-off-by: stack <stack@apache.org>
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
Feb 24, 2021
…java.io.IOException: Invalid HFile block magic" (apache#2882) Signed-off-by: Ramkrishna <ramkrishna@apache.org> Signed-off-by: stack <stack@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…java.io.IOException: Invalid HFile block magic"