HBASE-23679 Use new FileSystem objects during bulk loads #1019
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
Undoes lots of fanciness about FileSystem caching because of an explicit bug that was present (FileSystem only closed on one out of N regionservers, and not every time), but also because the FS caching itself is dodgy and prone to error. Each BulkLoad request will now get its own FileSystem instance that it is responsible for closing.
Looks like this isn't quite sufficient. Another leak (albeit, much slower) coming here. Need to do more to push down that DFS instance we made and use that until we move the files into the final location. Added some debug to FileSystem.java to see the above. Testing is just done via IntegrationTestBulkLoad with high number of loops but small chain length. |
|
💔 -1 overall
This message was automatically generated. |
|
Ugh, there's a few of these where, down in HStore, HRegion, and even the WAL code (ugh), which is all invoked via bulk load where we do a The fix is to just push down the FileSystem or use one that is already created, but this gets tricky in some places. Will need to step back from this all and see if there's a better way to do this. |
|
Thinking about this before I leave it for the night -- my initial thought was that as long as, in SecureBulkLoadManager, we managed the lifecycle of our FileSystem correctly, we'd be fine. But, the reality is that other parts of HBase are (inadvertently) relying on a FileSystem being cached. The smaller fix may be to re-add the caching, keep the reference counting, but correct the clean-up logic. |
|
Closing in favor of #1029 |
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.
Undoes lots of fanciness about FileSystem caching because of
an explicit bug that was present (FileSystem only closed on one
out of N regionservers, and not every time), but also because
the FS caching itself is dodgy and prone to error.
Each BulkLoad request will now get its own FileSystem instance
that it is responsible for closing.
The change to HRegion is because ${hbase.rootdir}/data is
chmod 700which means that a normal user cannot get the size of those files (you'll see lots of AccessDenied exceptions in the RS log). Using HRegionFilesystem instead keeps this from being an issue (reading the filesize as HBase instead of the user).