-
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-25711 Setting wrong data block encoding through ColumnFamilyDes… #3124
base: master
Are you sure you want to change the base?
Conversation
…criptorBuilder#setValue leading to servers down(Rajeshbabu)
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
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.
Tricky problem, but seems like a nice improvement.
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ColumnFamilyDescriptorBuilder.java
Outdated
Show resolved
Hide resolved
ATTRIBUTE_VALIDATOR.put(IN_MEMORY_COMPACTION_BYTES, | ||
(im) -> MemoryCompactionPolicy.valueOf(im.toString())); | ||
ATTRIBUTE_VALIDATOR.put(COMPRESSION_COMPACT_BYTES, | ||
(ca) -> Compression.Algorithm.valueOf(ca.toString())); |
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.
Doing the client-side validation is an improvement over what we have today. Can we do a server-side validation check as well?
For example, if a client is using new libraries than the server does, and (hypothetically) the client knows about some newer enum value that the server doesn't. A client-side check would fail first.
Should be pretty straightforward to add a new step into CreateTableProcedure and ModifyTableProcedure to check the HTD the user provides.
hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CreateTableProcedure.java
Lines 280 to 286 in 202b17f
private void preCreate(final MasterProcedureEnv env) | |
throws IOException, InterruptedException { | |
if (!getTableName().isSystemTable()) { | |
ProcedureSyncWait.getMasterQuotaManager(env) | |
.checkNamespaceTableAndRegionQuota( | |
getTableName(), (newRegions != null ? newRegions.size() : 0)); | |
} |
hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java
Lines 287 to 328 in 202b17f
private void prepareModify(final MasterProcedureEnv env) throws IOException { | |
// Checks whether the table exists | |
if (!env.getMasterServices().getTableDescriptors().exists(getTableName())) { | |
throw new TableNotFoundException(getTableName()); | |
} | |
// check that we have at least 1 CF | |
if (modifiedTableDescriptor.getColumnFamilyCount() == 0) { | |
throw new DoNotRetryIOException("Table " + getTableName().toString() + | |
" should have at least one column family."); | |
} | |
// If descriptor check is enabled, check whether the table descriptor when procedure was | |
// submitted matches with the current | |
// table descriptor of the table, else retrieve the old descriptor | |
// for comparison in order to update the descriptor. | |
if (shouldCheckDescriptor) { | |
if (TableDescriptor.COMPARATOR.compare(unmodifiedTableDescriptor, | |
env.getMasterServices().getTableDescriptors().get(getTableName())) != 0) { | |
LOG.error("Error while modifying table '" + getTableName().toString() | |
+ "' Skipping procedure : " + this); | |
throw new ConcurrentTableModificationException( | |
"Skipping modify table operation on table '" + getTableName().toString() | |
+ "' as it has already been modified by some other concurrent operation, " | |
+ "Please retry."); | |
} | |
} else { | |
this.unmodifiedTableDescriptor = | |
env.getMasterServices().getTableDescriptors().get(getTableName()); | |
} | |
this.deleteColumnFamilyInModify = isDeleteColumnFamily(unmodifiedTableDescriptor, | |
modifiedTableDescriptor); | |
if (!unmodifiedTableDescriptor.getRegionServerGroup() | |
.equals(modifiedTableDescriptor.getRegionServerGroup())) { | |
Supplier<String> forWhom = () -> "table " + getTableName(); | |
RSGroupInfo rsGroupInfo = MasterProcedureUtil.checkGroupExists( | |
env.getMasterServices().getRSGroupInfoManager()::getRSGroup, | |
modifiedTableDescriptor.getRegionServerGroup(), forWhom); | |
MasterProcedureUtil.checkGroupNotEmpty(rsGroupInfo, forWhom); | |
} | |
} |
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.
TableDescriptorChecker handling most of the validations but checking for valid data block encoding is missed so added that so it should be fine both create and modify cases.
@@ -282,6 +283,8 @@ | |||
|
|||
private final static Set<Bytes> RESERVED_KEYWORDS = new HashSet<>(); | |||
|
|||
private final static Map<Bytes,Function<Bytes,Object>> ATTRIBUTE_VALIDATOR = new HashMap<>(); |
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.
If we want to do this same validation server-side, it would be nice to life the map and the validator functions out to their own class which can be more easily re-used.
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.
Moved those to separate class.
@joshelser Thanks for review please review updated changes. Thanks. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
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.
Two very minor requests. Sorry it's so delayed.
LOG.error(key.toString() + " attribute value "+ (value != null ? value.toString() : "") | ||
+ " is not valid.", e); |
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.
nit: use the {}
notation to avoid a toString()
penalty here
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.
done.
hbase-client/src/main/java/org/apache/hadoop/hbase/util/ColumnFamilyAttributeValidator.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
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 on QA. Thanks for addressing my suggestions, Rajeshbabu!
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
retest build |
…criptorBuilder#setValue leading to servers down(Rajeshbabu)