> On Jan. 8, 2019, 9:44 p.m., Vihang Karajgaonkar wrote: > > 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.
I have changed it to avoid using get_table. Can you take a look? > On Jan. 8, 2019, 9:44 p.m., Vihang Karajgaonkar wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Lines 217 (patched) > > <https://reviews.apache.org/r/69585/diff/7/?file=2117531#file2117531line217> > > > > 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. good point. I was thinking all configuration is the same. but it is better to let each MetaStore instance has its own value. > On Jan. 8, 2019, 9:44 p.m., Vihang Karajgaonkar wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Lines 682 (patched) > > <https://reviews.apache.org/r/69585/diff/7/?file=2117531#file2117531line682> > > > > 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. This is avoided in new update > On Jan. 8, 2019, 9:44 p.m., Vihang Karajgaonkar wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Lines 4610 (patched) > > <https://reviews.apache.org/r/69585/diff/7/?file=2117531#file2117531line4610> > > > > 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. We need this call to authorize the partition access only when user can access its table. Sentry does not do any filtering at partition level. 1) With this approach, 1.1) sentry get privilege info from DB and authorize its table. 1.1.1) If allowed, HMS gets partition from DB, and sentry just returns input 1.1.2) If not allowed, throw exception. 2) Don't filter on table in HMS, but sentry filter on partition in filter hook (Your sugestion) 2.1) HMS gets partition object from DB, then call sentry filter on partition 2.2) Sentry gets privilege info from DB and authorize its table 2.2.1) If allowed, Sentry just returns input (sentry does not filter on partition itself) 2.2.2) If not allowed, throw exception. As you can see, approach 1) has less overhead when user cannot access that table. I have changed the implementation of authorizeTableForPartitionMetadata, so it does not get table object from HMS db. - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69585/#review211768 ----------------------------------------------------------- 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 > >
