----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69585/#review211843 -----------------------------------------------------------
Fix it, then Ship it! standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 652-655 (patched) <https://reviews.apache.org/r/69585/#comment297430> I think this is misleading since this assumes that DefaultMetaStoreFilterHook will not add anything in the future too. We should either depend only on METASTORE_SERVER_FILTER_ENABLED and run whichever filterHook is configured or throw an error when METASTORE_SERVER_FILTER_ENABLED is true but FILTER_HOOK is empty or DefaultMetaStoreFilterHook instead of silently disabling the filtering logic. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 661 (patched) <https://reviews.apache.org/r/69585/#comment297428> I didn't realize that FILTER_HOOK takes only one value not a comma separated list of classnames. In that case, this code can be made simpler using existing util methods Class<? extends MetaStoreFilterHook> clazz = JavaUtils.getClass(MetastoreConf.getVar(conf, ConfVars.FILTER_HOOK), MetaStoreFilterHook.class); return JavaUtils.getInstance(clazz, new Class<?>[] {Configuration.class}, new Object[] {conf}); standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestFilterHooks.java Lines 254 (patched) <https://reviews.apache.org/r/69585/#comment297431> Its not clear why this should throw NoSuchObjectException? Can you please add a comment. Are we changing the behavior of this API? standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetastoreFilterHook.java Lines 49 (patched) <https://reviews.apache.org/r/69585/#comment297432> This test looks very familiar to TestFilterHooks. What is the difference? If it is different, can you please add some javadoc on the top to explain what the test is doing. If there is not much difference can we refactor (or add these tests to TestFilterHooks) to re-use the code instead of duplicating it? - Vihang Karajgaonkar On Jan. 10, 2019, 9:10 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69585/ > ----------------------------------------------------------- > > (Updated Jan. 10, 2019, 9:10 p.m.) > > > Review request for hive, Adam Holley, Karthick Sankarachary, Morio > Ramdenbourg, Peter Vary, Sergio Pena, and Vihang Karajgaonkar. > > > Bugs: HIVE-20776 > https://issues.apache.org/jira/browse/HIVE-20776 > > > Repository: hive-git > > > Description > ------- > > add filtering to read result at HMS server, so user cannot see metadata > he/she has no privileges. Filtering is enabled/disabled based on > configuration. > > > Diffs > ----- > > > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java > 748b56b0a268c1ec7dea022722478ec50889c016 > > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java > be1f8c78497fe3d0816ad3935ba07cd5ad379b08 > > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/FilterUtils.java > PRE-CREATION > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > a9398ae1e79404a15894aa42f451df5d18ed3e4c > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestFilterHooks.java > 7dc69bc4e92875c8962dcd313b16f0f90ea8b057 > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetastoreFilterHook.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/69585/diff/8/ > > > Testing > ------- > > Existing unit tests passed. > add new unit tests for filtering at HMS server and HMS client > add code to enabled/disable filtering at HMS client based on configuration > add code to enabled/disable filtering at HMS server based on configuration > > > Thanks, > > Na Li > >
