Uh oh!
There was an error while loading. Please reload this page.
HBASE-24289 Heterogeneous Storage for Date Tiered Compaction - #1730
Merged
Conversation
pengmq1
commented
May 18, 2020
ContributorAuthor
Apache-HBase
commented
May 18, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 18, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 18, 2020
🎊 +1 overall
This message was automatically generated. |
busbey
commented
May 18, 2020
Contributor
Can you get the scope document into a markdown file in |
Apache-HBase
commented
May 19, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 19, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 19, 2020
🎊 +1 overall
This message was automatically generated. |
pengmq1
commented
May 25, 2020
ContributorAuthor
OK, I add it to |
Apache-HBase
commented
May 25, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 25, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 25, 2020
💔 -1 overall
This message was automatically generated. |
infraio
reviewed
May 26, 2020
| avoid moving data blocks at the HDFS level, we can set the file parent directory to | ||
| the storage policy we need before writing data. The new file automatically inherits the | ||
| storage policy of the parent directory, and is written according to the correct disk | ||
| type when writing. So as to avoid later data movement. |
| |hbase.hstore.compaction.date.tiered.cold.window.age.millis|Long.MAX|| | ||
| |hbase.hstore.compaction.date.tiered.cold.window.storage.policy|HOT|| | ||
| The original date tiered compaction related configuration has the same meaning and maintains |
Contributor
There was a problem hiding this comment.
What will happen if the CF config storage policy and enable this feature too?
ContributorAuthor
There was a problem hiding this comment.
If hbase.hstore.compaction.date.tiered.storage.policy.enable is true, this will override CF config storage policy, and hbase.hstore.block.storage.policy does not work. Because storefile must belong to one window and will use window storage policy
| } else { | ||
| writer = writerFactory.createWriter(); | ||
| } | ||
| //writer = writerFactory.createWriter(); |
| } | ||
| CommonFSUtils.setStoragePolicy(this.fs, dir, fileStoragePolicy); | ||
| if (LOG.isDebugEnabled()) { | ||
| LOG.debug( |
Contributor
There was a problem hiding this comment.
Only log once when region created? If so, can use info log.
ContributorAuthor
There was a problem hiding this comment.
yes, I think each type storage policy will create tmp dir once. CommonFSUtils.setStoragePolicy(this.fs, dir, fileStoragePolicy); should follow HRegionFileSystem.mkdirs?
Apache-HBase
commented
Jun 2, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 2, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 2, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 8, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 8, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 8, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 8, 2020
💔 -1 overall
This message was automatically generated. |
Apache9
approved these changes
Jun 21, 2020
| private final boolean needEmptyFile; | ||
| private final Map<Long, String> lowerBoundariesPolicies; |
ContributorAuthor
There was a problem hiding this comment.
lowerBoundariesPolicies is HashMap
| throws IOException { | ||
| if (request instanceof DateTieredCompactionRequest) { | ||
| return compactor.compact(request, ((DateTieredCompactionRequest) request).getBoundaries(), | ||
| ((DateTieredCompactionRequest) request).getBoundariesPolicies(), |
Contributor
There was a problem hiding this comment.
Cast it to DateTieredCompactionRequest with a locl variable and then make use of the casted instance?
ContributorAuthor
There was a problem hiding this comment.
Yeah... I follow the use of ((DateTieredCompactionRequest) request).getBoundaries() previous line of code.
Contributor
There was a problem hiding this comment.
Use a local variable and no need to cast twice...
| // before version 3.3.0 (See HDFS-13209). Use child dir here is to make stored files | ||
| // satisfy the specific storage policy when writing. So as to avoid later data movement. | ||
| // We don't want to change whole temp dir to 'fileStoragePolicy'. | ||
| if (fileStoragePolicy != null && !fileStoragePolicy.isEmpty()) { |
Contributor
There was a problem hiding this comment.
Just use Strings.isNullOrEmpty in guava.
infraio
reviewed
Jun 27, 2020
| * @throws IOException with error | ||
| */ | ||
| @Test | ||
| public void incomingWindowHot() throws IOException { |
Contributor
There was a problem hiding this comment.
Method name start with "test"? incomingWindowHot => testIncomingWindowHot
infraio
reviewed
Jun 27, 2020
| String prefix = HConstants.STORAGE_POLICY_PREFIX; | ||
| for (Path newFile : newFiles) { | ||
| if (newFile.getParent().getName().startsWith(prefix)) { | ||
| CommonFSUtils.setStoragePolicy(fs.getFileSystem(), newFile, |
Contributor
There was a problem hiding this comment.
Set storage policy for a file? This is not work before hdfs 3.3.0?
ContributorAuthor
There was a problem hiding this comment.
newFiles是已经compact结束将要被rename到region目录下的文件，但它们的storage policy还未指定，文件storage policy属性保存在INode中。rename之后，未指定storage policy的文件自动继承新的父目录。在rename之前调用setStoragePolicy()是为了使数据和storage policy保持一致，避免storage policy因为继承发生变化。HDFS允许对已经close的文件设置storage policy, 并不会发生数据移动。而且newFiles的数据和临时父目录的storage policy已经是一致的了(因为在compact之前tmp下已创建了不同存储策略的临时目录)
newFiles are files that have been compacted and will be renamed to the region directory, but their storage policy has not been specified. The file storage policy attribute is saved in INode. After renaming, files that do not specify a storage policy automatically inherit the new parent directory. Before renaming call to setStoragePolicy() here is to keep the data consistent with the storage policy and avoid the storage policy changing due to inheritance. HDFS allows storage policy to be set on files that have been closed, and no data movement will occur. Moreover, the data of newFiles and the storage policy of tmp parent dir are already consistent (because tmp directories of different storage strategies have been created under tmp dir before compact).
infraio
reviewed
Jun 27, 2020
| long[] sizes = new long[] { 30, 31, 32, 33, 34, 20, 21, 22, 23, 24, 25, 10, 11, 12, 13 }; | ||
| Map<Long, String> expected = new HashMap<>(); | ||
| // boundaries = { Long.MIN_VALUE, 12 } | ||
| expected.put(12L, HOT_WINDOW_SP); |
Contributor
There was a problem hiding this comment.
Explain more about the unit test? What is the different between this and other tests? And there are some duplicate code in these test methods?
Apache-HBase
commented
Jun 28, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 28, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 29, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 29, 2020
🎊 +1 overall
This message was automatically generated. |
asfgit pushed a commit
that referenced
this pull request
Jun 30, 2020
Signed-off-by: Guanghao Zhang <zghao@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
infraio pushed a commit
to infraio/hbase
that referenced
this pull request
Aug 17, 2020
…1730) Signed-off-by: Guanghao Zhang <zghao@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
clarax pushed a commit
to clarax/hbase
that referenced
this pull request
Nov 15, 2020
…1730) Signed-off-by: Guanghao Zhang <zghao@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
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
5 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.