HBASE-23095 Reuse FileStatus in StoreFileInfo #674
Merged
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
Apache9
requested changes
Sep 30, 2019
| @@ -152,7 +166,7 @@ public StoreFileInfo(final Configuration conf, final FileSystem fs, final Path i | |||
| */ | |||
| public StoreFileInfo(final Configuration conf, final FileSystem fs, final FileStatus fileStatus) | |||
| throws IOException { | |||
| this(conf, fs, fileStatus.getPath()); | |||
There was a problem hiding this comment.
OK, this is the problem...
| } | ||
|
|
||
| private StoreFileInfo(final Configuration conf, final FileSystem fs, final Path initialPath, | ||
| final Long createdTimestamp, final Long size) throws IOException { |
There was a problem hiding this comment.
What about just passing a FileStatus in?
There was a problem hiding this comment.
I initially thought the same Duo like directly passing "FileStatus" in constructor but what I feel that "Path" is also in same constructor and even that Path can be constructed from FileStatus. So that looks to me not right. :) Another important is Path can not be removed from constructor because many callers pass only Path object, not FileStatus.
I am fine Duo if you still want to change?
public StoreFileInfo(final Configuration conf, final FileSystem fs, final Path initialPath, final FileStatus filestatus)
There was a problem hiding this comment.
I think it is fine, as the FileStatus could be null. And maybe you could add a @nullable annotation to indicate this.
There was a problem hiding this comment.
Passing the individual params Ts and size looks cleaner IMO. Anyways explained why selected that path.
There was a problem hiding this comment.
But this requires extra boxing/unboxing? And what if we need more parameters in the future...
There was a problem hiding this comment.
Any way is ok. As this was not in a hot path, even that Autoboxing thing was also ok. That 2 params passing looked more clean from the calling stand point. Anyway not so big concern. This looks clear too.
There was a problem hiding this comment.
Thank you @anoopsjohn for the confirmation.
@Apache9 Duo, I already did new commit for your suggestions. Please confirm which one you would like to keep the old or new change?
6f4baae
There was a problem hiding this comment.
Its ok.. Keep it as the way u have pushed as of now.. Its ok only.
There was a problem hiding this comment.
Sure @anoopsjohn. Thanks again!!
There was a problem hiding this comment.
Appreciate Duo @Apache9 if you get some time for review :)
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
anoopsjohn
approved these changes
Oct 1, 2019
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@Apache9 Can you please approve? It still be point "change requested". |
asfgit
pushed a commit
that referenced
this pull request
Oct 8, 2019
Signed-off-by: Anoop Sam John <[email protected]> Signed-off-by: Duo Zhang <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Oct 8, 2019
Signed-off-by: Anoop Sam John <[email protected]> Signed-off-by: Duo Zhang <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Oct 8, 2019
Signed-off-by: Anoop Sam John <[email protected]> Signed-off-by: Duo Zhang <[email protected]>
infraio
pushed a commit
to infraio/hbase
that referenced
this pull request
Aug 17, 2020
Signed-off-by: Anoop Sam John <[email protected]> Signed-off-by: Duo Zhang <[email protected]>
symat
pushed a commit
to symat/hbase
that referenced
this pull request
Feb 17, 2021
Signed-off-by: Anoop Sam John <[email protected]> Signed-off-by: Duo Zhang <[email protected]> (cherry picked from commit 66506a6) Change-Id: Idd7edd87d0d29024518b8928c1684106ec4f8cc3
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.
We found this performance issue when taking a snapshot on large MOB table where MOB compaction is disabled more than 3 months, is due to MOB data loss issue HBASE-22075.