HBASE-23584 cache filestatus in storefileinfo #3596
+180
−143
Conversation
This file contains hidden or 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. |
|
🎊 +1 overall
This message was automatically generated. |
saintstack
force-pushed
the
HBASE-23584
branch
from
286eafb to
34e1c8f
Compare
|
💔 -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. |
saintstack
force-pushed
the
HBASE-23584
branch
from
34e1c8f to
c13b3f0
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
saintstack
force-pushed
the
HBASE-23584
branch
from
c13b3f0 to
9bc85a4
Compare
|
🎊 +1 overall
This message was automatically generated. |
jojochuang
reviewed
Aug 19, 2021
Contributor
jojochuang
left a comment
jojochuang
left a comment
There was a problem hiding this comment.
just a nit. the rest looks good
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileInfo.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
saintstack
force-pushed
the
HBASE-23584
branch
from
9bc85a4 to
342b196
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
@jojochuang good by you? |
Contributor
|
@saintstack why is this PR opened against branch-2.3, not master branch? |
jojochuang
reviewed
Aug 25, 2021
|
|
||
| /** | ||
| * Sets the region coprocessor env. | ||
| * @param coprocessorHost |
Contributor
There was a problem hiding this comment.
unrelated change, and this parameter isn't removed from the method.
Contributor
Author
There was a problem hiding this comment.
The @param had no descriptive text so the javadoc is useless... a style warning we usually clean as we go. I can put it back if you want (Would rather not).
| status = fs.getFileStatus(initialPath); | ||
| } | ||
| long length = status.getLen(); | ||
| getFileStatus(); |
Contributor
There was a problem hiding this comment.
remove this line since it looks redundant?
Contributor
Author
There was a problem hiding this comment.
The call loads the file status... caches it. Let me at least add comment here since two of you didn't get what is happening here.... (There is a comment on the cachedFileStatus member explaining the mechanism but let me make it better)
Contributor
Author
There was a problem hiding this comment.
Sorry, it is redundant.... now I look at it. Let me fix.
anoopsjohn
reviewed
Aug 25, 2021
| private static final Logger LOG = LoggerFactory.getLogger(StoreFileInfo.class); | ||
| /** | ||
| * Cached fileStatus for file or if a link or a reference, then the fileStatus of the referred-to | ||
| * file. We cache filestatus rather than ask NN each time we need it. We set it after construction |
Contributor
There was a problem hiding this comment.
nit: "Rather than ask FS each time " ? Its not NN always :-)
Contributor
There was a problem hiding this comment.
Minor : Also can we move this member variable below the static members so that all instance members together? That is something we normally follow as coding style
Contributor
Author
There was a problem hiding this comment.
Thanks. Let me fix.
| /** | ||
| * @return FileStatus for the linked or referenced file or if not a reference, then the hfile | ||
| */ | ||
| private FileStatus loadFileStatus(final FileSystem fs) throws IOException { |
Contributor
There was a problem hiding this comment.
Can avoid passing 'fs' as we can refer that here from this.fs
| status = fs.getFileStatus(initialPath); | ||
| } | ||
| long length = status.getLen(); | ||
| getFileStatus(); |
| this.size = fStatus.getLen(); | ||
| } | ||
| // If a fileStatus, use it. It is pointed at the hfile, not at a link or reference. | ||
| this.cachedFileStatus = fileStatus != null? fileStatus: fs.getFileStatus(p); |
Contributor
There was a problem hiding this comment.
So only in case of this is an HFile (not a split ref file and link ), we will keep the cachedFileStatus.
Not sure in such cases what status it will be passing. Should be status of the ref file or link file not the referred one or linked one right.. Checking deeply that we wont break anything in that regard with this patch.
Contributor
Author
There was a problem hiding this comment.
Yeah, only safe to cache if filestatus is a direct reference at this point. For link or ref, need to wait till we are asked get the filestatus for the endpoint of link/ref before we go ahead and cache... will look again to be sure. thanks.
Contributor
Author
There was a problem hiding this comment.
I checked again and whats here is fine... we are only here if its a direct file path. There are cases where we might be passed a filestatus for a link or reference but we won't get to this point so won't cache them... which is a missed opportunity I think. Let me see if opportunity for early caching passed filestatus even if link or reference. Will be back (This is not my patch originally... trying to help land someone else's good idea..).
Contributor
There was a problem hiding this comment.
Yep.. This is really useful stuff specially when storage is remote cloud storage. Getting the FileStatus is not that cheap. And that too 2 or 3 times in the path of open an input stream over an HFile. This is more clear when we have many scans where switch from pread to stream read happens. There we will have to open the IS over HFile again. Some times the data that we are looking for, might be already there in Cache. The switch will happen and so open new FIS.. There these calls becoming the overhead. I did some profiling last weeks and it was evident .. BTW my version is bit old 2.1.x where even some recent optimization in the path of HFile IS open was missing. This is really useful stuff even though a small patch... :-) Good on you Stack.. I will also closely see and how max we can avoid the calls for getFileStatus. BTW on the Jira I put another idea also. That involves more work and useful.. The required API from hadoop common is available in some later versions only. So we might have to play with it a bit.. Once this patch is in, lets discuss that also.
Contributor
Author
There was a problem hiding this comment.
To be clear, this is not my idea/patch. Original was by @HuiHang-Yu / yuhuiyang [email protected] from a while back (They are the author on this patch)
Contributor
Author
Habit. Let me put up next PR against master. Thanks for reviews. |
saintstack
force-pushed
the
HBASE-23584
branch
from
342b196 to
2b781d2
Compare
saintstack
force-pushed
the
HBASE-23584
branch
from
2b781d2 to
842e0d7
Compare
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
Update addresses review comments. Also is more conservative than previous patches (including original posters') regards when to cache filestatus. Posted this patch against master branch too over at #3655 |
|
💔 -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. |
saintstack
force-pushed
the
HBASE-23584
branch
from
842e0d7 to
c03b4a0
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
anoopsjohn
reviewed
Sep 7, 2021
Contributor
anoopsjohn
left a comment
anoopsjohn
left a comment
There was a problem hiding this comment.
Mostly looking good but have few more doubts Stack.
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileInfo.java
Show resolved
Hide resolved
| } | ||
| } | ||
| throw exToThrow; | ||
| status = this.link.getFileStatus(this.fs); |
Contributor
There was a problem hiding this comment.
The file status to be fetched from this.fs? Should be from passed param 'fs' ?
And all subsequent calls? When it is calls within this class, anyways we pass this.fs as param only. So that part will be ok.
Contributor
There was a problem hiding this comment.
Have not checked fully when it calls from Snapshot area. Just asking Stack.
Contributor
Author
There was a problem hiding this comment.
To be safe, it should use the passed param. This method is used outside of this class. I looked at shutting it down but that would be bigger change. Let me update. Thanks @anoopsjohn
Contributor
Author
There was a problem hiding this comment.
I closed down the access on this method. Changed snapshot so it creates a StoreFileInfo of its own if different filesystem. Cleaner. Thanks @anoopsjohn
saintstack
force-pushed
the
HBASE-23584
branch
from
c03b4a0 to
908f8aa
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
anoopsjohn
approved these changes
Sep 13, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Refresh of pr1380. Trying to land a nice idea that went stale in our PR queue. Kept author. Fixed styling.