----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68827/#review209055 -----------------------------------------------------------
Good overall, some small things below. standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java Lines 917 (patched) <https://reviews.apache.org/r/68827/#comment293318> I think a better name would be something like truncateMapByKey. Can you guarantee that it always receives non-null map? Then you don't need to check for null. Since you don't care about values, can this be a generic map `<String, T>`? standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java Lines 918 (patched) <https://reviews.apache.org/r/68827/#comment293321> You already have this check when you call this function. standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java Lines 928 (patched) <https://reviews.apache.org/r/68827/#comment293320> Looks like this can be a map with any value. standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java Lines 931 (patched) <https://reviews.apache.org/r/68827/#comment293319> Naming - may be something like 'truncateMapByKeys'? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/messaging/json/JSONMessageFactory.java Line 297 (original), 310 (patched) <https://reviews.apache.org/r/68827/#comment293325> The default exclude filter is empty. When someone sets this variable they should understand what they are doing. standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/common/TestMetaStoreUtils.java Lines 37 (patched) <https://reviews.apache.org/r/68827/#comment293324> Can you add a test with an empty exclude pattern? standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/common/TestMetaStoreUtils.java Lines 44 (patched) <https://reviews.apache.org/r/68827/#comment293322> It would be better to use hamcrest asserts and just check that you map matches your expected map with a single assert. - Alexander Kolbasov On Sept. 24, 2018, 8:37 p.m., Bharathkrishna Guruvayoor Murali wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68827/ > ----------------------------------------------------------- > > (Updated Sept. 24, 2018, 8:37 p.m.) > > > Review request for hive and Alexander Kolbasov. > > > Repository: hive-git > > > Description > ------- > > Clients can add large-sized parameters in Table/Partition objects. So we need > to enable adding regex patterns through HiveConf to match parameters to be > filtered from table and partition objects before serialization in HMS > notifications. > > > Diffs > ----- > > > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java > 30ea7f8129 > > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java > c681a87a1c > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/messaging/json/JSONMessageFactory.java > 2668b05320 > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/common/TestMetaStoreUtils.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/68827/diff/1/ > > > Testing > ------- > > > Thanks, > > Bharathkrishna Guruvayoor Murali > >
