----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69585/#review212147 -----------------------------------------------------------
Fix it, then Ship it! Overall the patch looks good. I have some minor suggestions below. RLGTM. standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/FilterUtils.java Lines 36 (patched) <https://reviews.apache.org/r/69585/#comment297790> please add documentation for each method standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/FilterUtils.java Lines 236 (patched) <https://reviews.apache.org/r/69585/#comment297793> more descriptive message would be dbName is null. Same for line 251 tblName is null. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 628 (patched) <https://reviews.apache.org/r/69585/#comment297791> nit, could be simplified as filterHook = isServerFilterEnabled ? loadFilterHooks() : null; standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 668-671 (patched) <https://reviews.apache.org/r/69585/#comment297792> Isn't this redundant since you already checked for a valid configuration in getIfServerFilterenabled() method? A easier way would be to add Preconditions.checkState(!isBlank(MetastoreConf.getVar(conf, ConfVars.FILTER_HOOK))); standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 4731 (patched) <https://reviews.apache.org/r/69585/#comment297795> nit, remove "For improved performance". I am not very convinced that this is helping the performance. its okay to say "we'll check if the said db and table are to be filtered out, if so, then we won't proceed with querying the partitions." standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 4873 (patched) <https://reviews.apache.org/r/69585/#comment297794> nit, remove "For improved performance". I am not very convinced that this is helping the performance. its okay to say "we'll check if the said db and table are to be filtered out, if so, then we won't proceed with querying the partitions." standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 5682 (patched) <https://reviews.apache.org/r/69585/#comment297796> nit, remove "For improved performance". I am not very convinced that this is helping the performance. its okay to say "we'll check if the said db and table are to be filtered out, if so, then we won't proceed with querying the partitions." Same comment for other places with this comment. standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetastoreFilterHook.java Lines 52 (patched) <https://reviews.apache.org/r/69585/#comment297798> If this test class added more test coverage to TestFilterHooks test, I would suggest to move new tests in TestFilterHooks instead of removing TestFilterHooks and adding a new test class which copies all the code from TestFilterHooks. That way you don't lose the git history of TestFilterHooks unnecessarily. - Vihang Karajgaonkar On Jan. 18, 2019, 3:45 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69585/ > ----------------------------------------------------------- > > (Updated Jan. 18, 2019, 3:45 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 > 19bd9bac84c20f94ac819a80e3cc89e0dc66396d > > 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/10/ > > > 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 > >
