----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69585/#review211768 -----------------------------------------------------------
Thanks for the patch. Do we really need to introduce authorizeTableForPartitionMetadata in these API calls. For the common case, it can potentially degrade API performance. For instance, for fetching a single partition, we are now doing a get_table and then get_partition for the common case. I think if it is not related to the functionality of this patch, we should do it in a separate patch with more investigation on its impact. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 84 (patched) <https://reviews.apache.org/r/69585/#comment297369> redundant import? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 217 (patched) <https://reviews.apache.org/r/69585/#comment297377> If you want to initialize this member using init() it shouldn't be static since it relies on the conf object which is not static. Technically, there is a race-condition in this variable since it is being overwritten every time init() method is called with the instance specific conf object. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 649 (patched) <https://reviews.apache.org/r/69585/#comment297367> nit, formatting. The curly brace convention we follow is if () { blah; } Easiest way to fix these errors is to import the code-style formatter xml file from the dev-support/eclipse-styles.xml (works for intellij too) and let IDE to reformat the newly added code. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 663-676 (patched) <https://reviews.apache.org/r/69585/#comment297366> You can reuse a existing method which does this with some minor renaming of the method and variables. The implementation of MetaStoreServerUtils.getMetaStoreListeners is generic enough to be used to any class. We probably can just rename it to more generic like getInstancesFromClass for example. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 667-674 (patched) <https://reviews.apache.org/r/69585/#comment297370> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 682 (patched) <https://reviews.apache.org/r/69585/#comment297372> Is there a better way to do this? This method is introducing a additional db call for all the methods for the common case of users having the required permissions. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 3143-3145 (patched) <https://reviews.apache.org/r/69585/#comment297371> Shouldn't the FilterUtils.filterTables be used here for consistency? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 4610 (patched) <https://reviews.apache.org/r/69585/#comment297373> The original API is fetching only one partition, this method is not improving performance but rather degrading it since this would do a fetch table and fetch partition for the most common case. I think we should do this check only in case of fetching lots of partitions where the cost of doing one get_table call is relatively low compared to fetching lots of partitions. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 4655 (patched) <https://reviews.apache.org/r/69585/#comment297374> same comment as above. not sure if this method is helping much with the performance here. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 4683 (patched) <https://reviews.apache.org/r/69585/#comment297375> same comment as above. not sure if this method is helping much with the performance here. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 4711 (patched) <https://reviews.apache.org/r/69585/#comment297376> move this method call below checkLimitNumberOfPartitionsByFilter. standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetastoreFilterHook.java Lines 318 (patched) <https://reviews.apache.org/r/69585/#comment297378> Don't we need a boolean argument here too to confirm that only server side filter logic is tested? - Vihang Karajgaonkar On Jan. 8, 2019, 8:03 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69585/ > ----------------------------------------------------------- > > (Updated Jan. 8, 2019, 8:03 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/7/ > > > 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 > >
