HBASE-23584 : Descrease rpc getFileStatus count when open a storefile #958
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. |
saintstack
reviewed
Dec 24, 2019
| @@ -51,6 +51,7 @@ | |||
| public class StoreFileInfo { | |||
| private static final Logger LOG = LoggerFactory.getLogger(StoreFileInfo.class); | |||
|
|
|||
| private FileStatus localStatus; | |||
There was a problem hiding this comment.
I like idea of caching status. When does it get invalidated?
There was a problem hiding this comment.
Now it will be updated when reopened only because hfile will not be modified after writen i think . Is there some scenarios fileStatus will be modified?
There was a problem hiding this comment.
Not that I can think of.
| @@ -108,6 +109,7 @@ | |||
| private RegionCoprocessorHost coprocessorHost; | |||
|
|
|||
| // timestamp on when the file was created, is 0 and ignored for reference or link files | |||
| // the before timestamp is shown as above ! Now i change to use the createdTimestamp of reference or link files , will it create some problem later ? | |||
There was a problem hiding this comment.
Where is 'above'. Comments don't usually refer to 'i' since code is product of many. What is the problem this will create and if you don't know what it might be, why change i?
| @@ -296,7 +297,7 @@ ReaderContext createReaderContext(boolean doDropBehind, long readahead, ReaderTy | |||
| if (this.link != null) { | |||
| // HFileLink | |||
| in = new FSDataInputStreamWrapper(fs, this.link, doDropBehind, readahead); | |||
| status = this.link.getFileStatus(fs); | |||
| // status = this.link.getFileStatus(fs); | |||
There was a problem hiding this comment.
Remove rather than comment-out.
There was a problem hiding this comment.
Oh my fault ! I will remove it later !
| @@ -370,13 +372,16 @@ private HDFSBlocksDistribution computeHDFSBlocksDistributionInternal(final FileS | |||
| */ | |||
| public FileStatus getReferencedFileStatus(final FileSystem fs) throws IOException { | |||
| FileStatus status; | |||
| if(this.localStatus !=null) return this.localStatus; | |||
There was a problem hiding this comment.
Need parens as per convention in the code that surrounds this. Needs space after the '='.
| @@ -51,6 +51,7 @@ | |||
| public class StoreFileInfo { | |||
| private static final Logger LOG = LoggerFactory.getLogger(StoreFileInfo.class); | |||
|
|
|||
| private FileStatus localStatus; | |||
There was a problem hiding this comment.
Does it need to be volatile? Will StoreFileInfo be accessed by many threads? Can it be final/created on construction?
| return link.getFileStatus(fs); | ||
| this.localStatus = link.getFileStatus(fs); | ||
| status = this.localStatus; | ||
| return status; |
There was a problem hiding this comment.
This code is repeated. Put it into a method?
| } catch (FileNotFoundException ex) { | ||
| // try the other location | ||
| exToThrow = ex; | ||
| } | ||
| } | ||
| throw exToThrow; | ||
| } else { | ||
| status = fs.getFileStatus(initialPath); | ||
| this.localStatus = fs.getFileStatus(initialPath); | ||
| status = this.localStatus; |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
saintstack
reviewed
Dec 27, 2019
| @@ -107,7 +108,7 @@ | |||
|
|
|||
| private RegionCoprocessorHost coprocessorHost; | |||
|
|
|||
| // timestamp on when the file was created, is 0 and ignored for reference or link files | |||
| // change to use the createdTimestamp of reference or link files , will it create some problem later ? | |||
There was a problem hiding this comment.
Is this comment correct? The createTimeStamp is gotten once only on construction. YOu are not changing the behavior that I can see. Should createTimestamp be made final as in private final long createdTimestamp?
There was a problem hiding this comment.
It is not necessary to make createdTimestamp to be final i think .
There was a problem hiding this comment.
Its good to mark data members final if your intent is that they are not ever to change. It helps the compiler and it helps the dev reading the code later.
| FileStatus fStatus = fs.getFileStatus(initialPath); | ||
| this.createdTimestamp = fStatus.getModificationTime(); | ||
| this.size = fStatus.getLen(); | ||
| this.createdTimestamp = this.getFileStatus().getModificationTime(); |
There was a problem hiding this comment.
Generally, we do not put the 'this.' in front of local method invocation.
| /** | ||
| * Get the {@link FileStatus} of the file referenced by this StoreFileInfo | ||
| * @param fs The current file system to use. | ||
| * @return The {@link FileStatus} of the file referenced by this StoreFileInfo | ||
| */ | ||
| public FileStatus getReferencedFileStatus(final FileSystem fs) throws IOException { | ||
| FileStatus status; | ||
| if(this.localStatus != null) {return this.localStatus;} |
There was a problem hiding this comment.
Do you think multiple threads will come in h ere at same time? Maybe it would be better to make localStatus be final and assign it on construction of this StoreFileInfo? If so, change name localStatus to status?
There was a problem hiding this comment.
LocalStatus will be assigned when storeFileInfo construction is invoked . Will it be some problem happend when multiple threads will come in here at the same time ? So it is not necessary to make localstatus to be final and volatile i think .
There was a problem hiding this comment.
If so, make it final then we are explicit that it does not change during life time of a StoreFileInfo instance and we are clear about its thread safety. If done in constructor, you don't need this null check then either?
There was a problem hiding this comment.
Couldn't you simplify this method if you fetched status on construction (using the bulk of this method but taking it private/internal?)?
HorizonNet
reviewed
Dec 30, 2019
| @@ -362,21 +360,23 @@ private HDFSBlocksDistribution computeHDFSBlocksDistributionInternal(final FileS | |||
| return FSUtils.computeHDFSBlocksDistribution(fs, status, 0, status.getLen()); | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Seems like there is a little space added. Could you please remove it?
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
saintstack
reviewed
Jan 3, 2020
| @@ -107,7 +108,7 @@ | |||
|
|
|||
| private RegionCoprocessorHost coprocessorHost; | |||
|
|
|||
| // timestamp on when the file was created, is 0 and ignored for reference or link files | |||
| // change to use the createdTimestamp of reference or link files , will it create some problem later ? | |||
There was a problem hiding this comment.
Its good to mark data members final if your intent is that they are not ever to change. It helps the compiler and it helps the dev reading the code later.
| FileStatus fStatus = fs.getFileStatus(initialPath); | ||
| this.createdTimestamp = fStatus.getModificationTime(); | ||
| this.size = fStatus.getLen(); | ||
| this.createdTimestamp = getFileStatus().getModificationTime(); |
There was a problem hiding this comment.
Seems odd that we set modification time to created but that is not of your making; it was there before you came along (perhaps only modification 'works' or this is more trustworthy than create time...)
| /** | ||
| * Get the {@link FileStatus} of the file referenced by this StoreFileInfo | ||
| * @param fs The current file system to use. | ||
| * @return The {@link FileStatus} of the file referenced by this StoreFileInfo | ||
| */ | ||
| public FileStatus getReferencedFileStatus(final FileSystem fs) throws IOException { | ||
| FileStatus status; | ||
| if(this.localStatus != null) {return this.localStatus;} |
There was a problem hiding this comment.
Couldn't you simplify this method if you fetched status on construction (using the bulk of this method but taking it private/internal?)?
initialize localStatus on construction
|
💔 -1 overall
This message was automatically generated. |
2 similar comments
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
update 2020-03-30
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.
https://issues.apache.org/jira/browse/HBASE-23584