HBASE-27021 StoreFileInfo should set its initialPath in a consistent way #4419
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. |
joshelser
approved these changes
May 11, 2022
| @@ -119,18 +119,18 @@ public class StoreFileInfo implements Configurable { | |||
| */ | |||
| public StoreFileInfo(final Configuration conf, final FileSystem fs, final Path initialPath, | |||
| final boolean primaryReplica) throws IOException { | |||
| this(conf, fs, null, initialPath, primaryReplica); | |||
| this(conf, fs, fs.getFileStatus(initialPath), primaryReplica); | |||
There was a problem hiding this comment.
Should make sure constructor javadoc is updated as this is a Namenode call. If we call this often, we're incurring remote reads which could be a bad perf hit. Need to make sure current callers are behaving nicely (we should be caching StoreFileInfo's already)
There was a problem hiding this comment.
I think this is intentional, we want to reduce the request to namenode...
There was a problem hiding this comment.
Ok, haven't thought about that. Let me revert this, and sanitize the path, instead.
| sf = it.next(); | ||
| } | ||
| sf.closeStoreFile(true); | ||
| store.getStoreEngine().getStoreFileManager().removeCompactedFiles(Lists.newArrayList(sf)); |
There was a problem hiding this comment.
nit: Collections.singletonList(sf) might be a little less heavy than a new ArrayList (since you're changing this)
|
I think the key problem here is we use StoreFileInfo as the key of a hash map. In general, within a typical store, we just need to use the storefile's name to decide whether they are the same StoreFileInfo. Or at least, when comparing, we should normalize the Path object to the same format... |
|
OK, there is a FileSystem instance in the StoreFileInfo, so I think we just need to make a makeQualified to normalize the initial path? |
Yep, TestHStore was fine, but noticed TestStoreFileInfo failed here. Need to double check on that. |
Apache9
approved these changes
May 11, 2022
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
wchevreuil
added a commit
that referenced
this pull request
May 12, 2022
…way (#4419) Signed-off-by: Josh Elser <elserj@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
wchevreuil
added a commit
that referenced
this pull request
May 12, 2022
…way (#4419) Signed-off-by: Josh Elser <elserj@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…way (apache#4419) Change-Id: I3d8a68d73fb91a31e0c8c41f50bf2cc06ff00685 Signed-off-by: Josh Elser <elserj@apache.org> Signed-off-by: Duo Zhang <zhangduo@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.
No description provided.