Uh oh!
There was an error while loading. Please reload this page.
HBASE-26938 Introduce a StoreFileWriterCreationTracker - #4350
Merged
Conversation
Apache-HBase
commented
Apr 15, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 15, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 15, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 15, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 15, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 15, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 15, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 15, 2022
🎊 +1 overall
This message was automatically generated. |
joshelser
approved these changes
Apr 17, 2022
joshelser
left a comment
Member
There was a problem hiding this comment.
I was able to reproduce this locally with hbase pe randomWrite when setting the max filesize for the table down to 1GB to force a lot of splits with multiple compaction threads. Reran the same experiment and did not have the same problem with this patch. Looks like an effective fix.
It looks like we might be losing some test coverage in the compaction progress, but I didn't dig into the code to see if we do have coverage elsewhere for it. Would be good to make sure we have something, but that can be done later to fix this issue first. Thanks Duo.
| * {@link BrokenStoreFileCleaner} to prevent deleting the these files as they are not present in | ||
| * SFT yet. | ||
| */ | ||
| Set<Path> getWrittenStoreFiles() { |
Member
There was a problem hiding this comment.
Suggested change
| Set<Path> getWrittenStoreFiles() { | |
| Set<Path> getStoreFilesBeingCreated() { |
Maybe this instead? Trying to reflect in the name that they are still being written to. "getWrittenStoreFiles" could be read as "they were previously written to" not "actively being written to".
Contributor
There was a problem hiding this comment.
Or …filesBeingWritten, which echos Hadoop HDFS terminology.
ContributorAuthor
There was a problem hiding this comment.
I like filesBeingWritten. Let me change the naming. Thanks both for helping!
| // see if CompactionProgress has done its thing on at least one store | ||
| int storeCount = 0; | ||
| for (HStore store : r.getStores()) { | ||
| CompactionProgress progress = store.getCompactionProgress(); |
Member
There was a problem hiding this comment.
Do we have test coverage for getProgress() elsewhere?
Contributor
There was a problem hiding this comment.
This was not the appropriate place to test progress reporting in the first place, and progress reporting was broken anyway because it was not MT safe. I do think we should add coverage for it but it should be a follow up issue that focuses on the lack of coverage (it’s always been an issue imho)
ContributorAuthor
There was a problem hiding this comment.
We can file another issue for improving the test for compaction progress.
apurtell
approved these changes
Apr 17, 2022
apurtell
left a comment
Contributor
There was a problem hiding this comment.
Confirmed the fix with application of this patch and test with the scenario that revealed the issue.
Apache-HBase
commented
Apr 17, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 17, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 17, 2022
💔 -1 overall
This message was automatically generated. |
Apache9
commented
Apr 17, 2022
ContributorAuthor
Test failures are not related. Let me merge. |
vinayakphegde pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…pache#4350) Introduce a StoreFileWriterCreationTracker to track the store files being written Signed-off-by: Josh Elser <elserj@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> (cherry picked from commit 48c4a46) Change-Id: Ie824b762d4f729340c673cc68699dff286eedf16
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
Sign up for freeto 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.
No description provided.