[GitHub] [hbase] joshelser commented on a change in pull request #3124: HBASE-25711 Setting wrong data block encoding through ColumnFamilyDes…

[GitBox <gi...@apache.org>]

joshelser commented on a change in pull request #3124:
URL: https://github.com/apache/hbase/pull/3124#discussion_r607291323



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/ColumnFamilyDescriptorBuilder.java
##########
@@ -1376,5 +1404,16 @@ public ModifyableColumnFamilyDescriptor setStoragePolicy(String policy) {
       return setValue(STORAGE_POLICY_BYTES, policy);
     }
 
+    /**
+     * Validates values of HBase defined column family attributes.
+     * Throws {@link IllegalArgumentException} if the value of attribute is not proper format.
+     * @param key
+     * @param value
+     */
+    public void validateAttributeValue(Bytes key, Bytes value) {
+      if(ATTRIBUTE_VALIDATOR.containsKey(key)) {
+        ATTRIBUTE_VALIDATOR.get(key).apply(value);

Review comment:
       Would be nice to catch the formatting exception here and give a clear error as to what attribute failed to parse.

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/ColumnFamilyDescriptorBuilder.java
##########
@@ -305,6 +308,30 @@
     RESERVED_KEYWORDS.add(new Bytes(Bytes.toBytes(IS_MOB)));
     RESERVED_KEYWORDS.add(new Bytes(Bytes.toBytes(MOB_THRESHOLD)));
     RESERVED_KEYWORDS.add(new Bytes(Bytes.toBytes(MOB_COMPACT_PARTITION_POLICY)));
+    initValidator();
+  }
+
+  private static void initValidator() {
+    ATTRIBUTE_VALIDATOR.put(DATA_BLOCK_ENCODING_BYTES,
+      (d) -> DataBlockEncoding.valueOf(d.toString()));
+    ATTRIBUTE_VALIDATOR.put(COMPRESSION_BYTES,
+      (ca) -> Compression.Algorithm.valueOf(ca.toString()));
+    ATTRIBUTE_VALIDATOR.put(BLOOMFILTER_BYTES, (bl) -> BloomType.valueOf(bl.toString()));
+    ATTRIBUTE_VALIDATOR.put(REPLICATION_SCOPE_BYTES, (rs) -> Integer.valueOf(rs.toString()));
+    ATTRIBUTE_VALIDATOR.put(MIN_VERSIONS_BYTES, (min) -> Integer.valueOf(min.toString()));
+    ATTRIBUTE_VALIDATOR.put(MAX_VERSIONS_BYTES, (max) -> Integer.valueOf(max.toString()));
+    ATTRIBUTE_VALIDATOR.put(BLOCKSIZE_BYTES, (bs) -> Integer.valueOf(bs.toString()));
+    ATTRIBUTE_VALIDATOR.put(KEEP_DELETED_CELLS_BYTES,
+      (k) -> KeepDeletedCells.valueOf(k.toString()));
+    ATTRIBUTE_VALIDATOR.put(TTL_BYTES, (ttl) -> Integer.valueOf(ttl.toString()));
+    ATTRIBUTE_VALIDATOR.put(DFS_REPLICATION_BYTES, (dr) -> Integer.valueOf(dr.toString()));
+    ATTRIBUTE_VALIDATOR.put(MOB_THRESHOLD_BYTES, (mt) -> Long.valueOf(mt.toString()));
+    ATTRIBUTE_VALIDATOR.put(MOB_COMPACT_PARTITION_POLICY_BYTES,
+      (mc) -> MobCompactPartitionPolicy.valueOf(mc.toString()));
+    ATTRIBUTE_VALIDATOR.put(IN_MEMORY_COMPACTION_BYTES,
+      (im) -> MemoryCompactionPolicy.valueOf(im.toString()));
+    ATTRIBUTE_VALIDATOR.put(COMPRESSION_COMPACT_BYTES,
+      (ca) -> Compression.Algorithm.valueOf(ca.toString()));

Review comment:
       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.
   
   https://github.com/apache/hbase/blob/202b17f4fc3229e91d584837776d53ed3e2e8adb/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CreateTableProcedure.java#L280-L286
   
   https://github.com/apache/hbase/blob/202b17f4fc3229e91d584837776d53ed3e2e8adb/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java#L287-L328

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/ColumnFamilyDescriptorBuilder.java
##########
@@ -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<>();

Review comment:
       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.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org