----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69585/#review211465 -----------------------------------------------------------
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java Lines 1837 (patched) <https://reviews.apache.org/r/69585/#comment296625> Is this the only place where is called? Btw, the patch is meant fo the server-side only, we could enhance the client-side another patch to keep this simpler. standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java Lines 220-221 (patched) <https://reviews.apache.org/r/69585/#comment296623> These two variables are not necessary. To disable the filter, the admin can just remove the filter hooks from the configuration and voilà, filter disabled. This applies to both sides, server-side and client-side. standalone-metastore/metastore-server/pom.xml Lines 242-246 (patched) <https://reviews.apache.org/r/69585/#comment296624> Why is this dep necessary? I wonder if this conflicts with the standalone solution where all the Hive dependences were removed. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 1405 (patched) <https://reviews.apache.org/r/69585/#comment296626> I thought we were going to use the PreReadEvent instead. Also, I'm concerned the implementation of filterDatabase does not throw NoSuchObjectException, and instead returns null. We should check both cases to guarantee get_database and others methods will deny the access to this database. I think we should use the PreReadEvent which is meant for authorization hooks. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 2934 (patched) <https://reviews.apache.org/r/69585/#comment296627> What is this extra space? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 2910-2911 (original), 2943-2946 (patched) <https://reviews.apache.org/r/69585/#comment296628> Why is this change needed? I don't see anything new except splitting it in two lines. This will stay in the git history. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Line 2931 (original), 2966-2970 (patched) <https://reviews.apache.org/r/69585/#comment296629> Notice the firePreEvent before the filter. If we use that for authorization checks, then the filter is not required here. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 3050-3053 (patched) <https://reviews.apache.org/r/69585/#comment296630> Same question, why splitting the lines where if this patch doesn't need it? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 3016-3017 (original), 3059-3062 (patched) <https://reviews.apache.org/r/69585/#comment296631> Same question, why splitting the lines where if this patch doesn't need it? - Sergio Pena On Dec. 19, 2018, 10:50 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69585/ > ----------------------------------------------------------- > > (Updated Dec. 19, 2018, 10:50 p.m.) > > > Review request for hive, Adam Holley, 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 > 9eb1193a27120b5167f92daf67bf6a1c4e1d9927 > > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java > fb0b2fe6fb9fd4b4c92a6a39f06f39a4641aaabd > standalone-metastore/metastore-server/pom.xml > 895abfc423f00b121ee63e40904f5b3e57aea8ed > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > 0a1b96dcf62d3536cab2ce074d27a6225b2d3443 > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetastoreFilterHook.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/69585/diff/3/ > > > 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 > > > Thanks, > > Na Li > >
