-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-28447 New site configuration option "hfile.block.size" #5820
base: master
Are you sure you want to change the base?
Conversation
Introduce a new configuration setting - "hfile.block.size" that, if set, will define the default blocksize to use when writing HFiles if a column family schema does not define its own non-default block size. This is a bit complicated but required for compatability. The rules are: - If the schema specifies a non default block size, use it. - Otherwise, if the configuration specifies a non default block size, use it. - Otherwise, use the default block size. The default is defined by HConstants.DEFAULT_BLOCKSIZE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@Test | ||
public void testGetBlockSize() throws IOException { | ||
int eightK = 8 * 1024; | ||
int oneM = 1024 * 1024; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this could have been part of any existing test class, but no problem having it's own separate class either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's best to start a TestStoreUtils class for testing StoreUtils. Some other units might be moved in here or added later. That would make more sense to me.
💔 -1 overall
This message was automatically generated. |
The spotless check fails but when I run 'mvn spotless:apply' on my local branch there are no changes. |
Ah, there was something wrong with CI during the spotless check
|
Ya that happens sometimes, not sure why |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
No table level configuration? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overal LGTM - added one clarifying question.
* @param schemaBlockSize The block size as specified in the column family schema. | ||
* @return The block size to use. | ||
*/ | ||
public static int getBlockSize(Configuration conf, int schemaBlockSize) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A clarification :
Though you have already called out the "non default block size" - One Question - Right now there can be a case where the site configuration has non default block size say 1MB(to be applied to all the table/CF), and for some CF/table one wants explicitly 64 KB - so setting 64KB explicitly again in the schema won't be picked, as that is the default.
For such cases, one will need to explicitly set the schemaBlocksize(either BLOCKSIZE or configuration override in schema) for all required tables to 1MB.
@apurtell is this ready to be merged? It currently has fixVersion 2.6.0, and I'm looking to create the next RC tomorrow |
Introduce a new configuration setting - "hfile.block.size" that, if set, will define the default blocksize to use when writing HFiles if a column family schema does not define its own non-default block size. This is a bit complicated but required for compatability. The rules are:
The default is defined by HConstants.DEFAULT_BLOCKSIZE.
Given how compound configurations work the precedence order for a non default block size is: BLOCKSIZE in the column family schema > "hfile.block.size" in CF or table level schema > "hfile.block.size" in site configuration > HConstants.DEFAULT_BLOCKSIZE