Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/#review212157
---




standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/client/TestListPartitions.java
Line 1290 (original), 1290 (patched)


I change the expected exception type to be consistent with 
testListPartitionsAllNoDbName().

Same for test cases for NoTblName


- Na Li


On Jan. 18, 2019, 9:30 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, 9:30 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/client/TestListPartitions.java
>  a338bd4032eb746cd541a562ead0fa2caf9e3ed4 
> 
> 
> Diff: https://reviews.apache.org/r/69585/diff/11/
> 
> 
> 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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/
---

(Updated Jan. 18, 2019, 9:30 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 (updated)
-

  
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/client/TestListPartitions.java
 a338bd4032eb746cd541a562ead0fa2caf9e3ed4 


Diff: https://reviews.apache.org/r/69585/diff/11/

Changes: https://reviews.apache.org/r/69585/diff/10-11/


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

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Vihang Karajgaonkar via Review Board]

---
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)


please add documentation for each method



standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/FilterUtils.java
Lines 236 (patched)


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)


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)


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)


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)


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)


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)


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 
>   
> 

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

---
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 (updated)
-

  
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/

Changes: https://reviews.apache.org/r/69585/diff/9-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

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

> On Jan. 10, 2019, 11:20 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestFilterHooks.java
> > Lines 254 (patched)
> > 
> >
> > Its not clear why this should throw NoSuchObjectException? Can you 
> > please add a comment. Are we changing the behavior of this API?

We are not changing the behavior of this API. The original behavior could throw 
NoSuchObjectException as well. In this specific test, NoSuchObjectException was 
not thrown when user cannot access a table without my patch because we did not 
configure pre event listener (which checks if the user can access the 
partition's table, and throw NoSuchObjectException when user cannot access 
partition's table). NoSuchObjectException is thrown now because we add checking 
the filter hook, and NoSuchObjectException is thrown when user cannot access a 
table


> On Jan. 10, 2019, 11:20 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetastoreFilterHook.java
> > Lines 49 (patched)
> > 
> >
> > 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?

I remove TestFilterHooks since its test cases are subset of what are in 
TestHiveMetastoreFilterHook


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/#review211843
---


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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/
---

(Updated Jan. 17, 2019, 10:33 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 (updated)
-

  
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/9/

Changes: https://reviews.apache.org/r/69585/diff/8-9/


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

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Vihang Karajgaonkar via Review Board]

---
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)


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)


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 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)


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)


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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

---
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 (updated)
-

  
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/

Changes: https://reviews.apache.org/r/69585/diff/7-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

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

> On Jan. 8, 2019, 9:44 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetastoreFilterHook.java
> > Lines 318 (patched)
> > 
> >
> > Don't we need a boolean argument here too to confirm that only server 
> > side filter logic is tested?

No need. Some functions do not call filtering hook, so we cannot call it to 
test filter hook at HMS server. That is why we need input for server or client 
to decide if we should skip some cases.
For partition, all functions call filtering hook, so we don't need to skip some 
cases for testing filter hook at server.


- 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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

> 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 4711 (patched)
> > 
> >
> > move this method call below checkLimitNumberOfPartitionsByFilter.

good point!


- 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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

> 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 4655 (patched)
> > 
> >
> > same comment as above. not sure if this method is helping much with the 
> > performance here.

same as above explaination


- 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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

> 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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
>  

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Vihang Karajgaonkar via Review Board]

---
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)


redundant import?



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 217 (patched)


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)


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)


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)






standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 682 (patched)


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)


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)


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)


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)


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)


move this method call below checkLimitNumberOfPartitionsByFilter.



standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetastoreFilterHook.java
Lines 318 (patched)


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/
> 

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

---
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 (updated)
---

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

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Adam Holley via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/#review211767
---




standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
Lines 286-290 (patched)


Could be simplified to:
LOG.info("HMS client filtering is " + (isEnabled?"enabled.":"disabled."))


- Adam Holley


On Jan. 7, 2019, 10:26 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69585/
> ---
> 
> (Updated Jan. 7, 2019, 10:26 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
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/
---

(Updated Jan. 4, 2019, 4:07 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 (updated)
-

  
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/

Changes: https://reviews.apache.org/r/69585/diff/6-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


Thanks,

Na Li

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/
---

(Updated Dec. 31, 2018, 6:54 a.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 (updated)
-

  
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
 eb95e129a1a69fdbf9304676f8e8d97c06d48dea 
  
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/6/

Changes: https://reviews.apache.org/r/69585/diff/5-6/


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

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

> On Dec. 19, 2018, 11:33 p.m., Morio Ramdenbourg wrote:
> > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
> > Lines 662 (patched)
> > 
> >
> > The second parameter in ConfVars corresponds to the Hive property name. 
> > 
> > E.g. metastore.client.filter.result -> 
> > hive.metastore.client.filter.result
> > 
> > So this should be: 
> > METASTORE_CLIENT_FILTER_RESULT("metastore.client.filter.result", 
> > "hive.metastore.client.filter.result" ...)
> 
> Na Li wrote:
> this is new field, and does not exist in hive configuration. Since we are 
> splitting HMS from Hive, does it make sense to add this new one in hive 
> configuration?
> 
> Morio Ramdenbourg wrote:
> Hmm, I'm not exactly sure - HMS is still part of the Hive code base.
> 
> Others who have been adding new properties to only HMS have been putting 
> Hive name, so I think it's safe. Someone should confirm this though.
> 
> Na Li wrote:
> I see both examples in MetastoreConf.java. I will talk with others to 
> find out the rule.
> 
> 1) hiveName is "hive." + varname
> ADDED_JARS("metastore.added.jars.path", "hive.added.jars.path", "",
> "This an internal parameter."),
> 
> 
> 2) hiveName is the same as varname
> CATALOG_DEFAULT("metastore.catalog.default", 
> "metastore.catalog.default", "hive",
> "The default catalog to use when a catalog is not specified.  
> Default is 'hive' (the " +
> "default catalog)."),

I changed the hiveName to start with "hive."


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/#review211446
---


On Dec. 22, 2018, 12:27 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69585/
> ---
> 
> (Updated Dec. 22, 2018, 12:27 a.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-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
>  0a1b96dcf62d3536cab2ce074d27a6225b2d3443 
>   
> 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/5/
> 
> 
> 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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/
---

(Updated Dec. 22, 2018, 12:27 a.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 (updated)
-

  
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-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
 0a1b96dcf62d3536cab2ce074d27a6225b2d3443 
  
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/5/

Changes: https://reviews.apache.org/r/69585/diff/4-5/


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

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

> On Dec. 19, 2018, 11:33 p.m., Morio Ramdenbourg wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 4621 (patched)
> > 
> >
> > This is the same method as the one on the HiveMetaStoreClient. Would it 
> > be possible to move it to a common method under 
> > hive-standalone-metastore-common?
> 
> Na Li wrote:
> This function authorize access to the table of the partitions. Sergion is 
> going to implement this when filtering partitions in SENTRY-2481. So I will 
> remove this function in HMS client and server

I moved it to common untility class


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/#review211446
---


On Dec. 21, 2018, 9:36 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69585/
> ---
> 
> (Updated Dec. 21, 2018, 9:36 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-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
>  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/4/
> 
> 
> 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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

> On Dec. 20, 2018, 2:46 p.m., Sergio Pena wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 1405 (patched)
> > 
> >
> > 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.
> 
> Na Li wrote:
> The PreReadEvent calls sentry MetastoreAuthzBindingBase for 
> authorization. As you can see in
> 
> https://github.com/apache/sentry/blob/master/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java#L169,
>  it does not do anything on reading metadata. Therefore, we have to use 
> filter to prevent user from reading metadata it does not have privileges. 
> 
> I can only remove the filter for get_database() or get_table() if we 
> change sentry's implementation for that file.

for get_table(), get_database(), get_parittion*(), I remove the filter check, 
and use firePreEvent() to verify that user can access the metadata.

When getting list of names, we only call filter hook to remove names that user 
should not see.


Accessing metadata of an object requires more privileges than just getting the 
names. For example, "CREATE" privilege on server allows listing DB names, so 
user does not create a DB using a name of an existing DB.  "CREATE" privilege 
on DB allows listing Table names. But "CREATE" privilege does not allow 
describe a particular table (show meta data of a table user has no privilege) 
regardless it is for authorization or filtering. So we should only check 
authorization (firePreEvent()) for single item like get_database() or 
get_table(). For getting a list of item names, we only call filter hook.


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/#review211465
---


On Dec. 21, 2018, 9:36 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69585/
> ---
> 
> (Updated Dec. 21, 2018, 9:36 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-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
>  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/4/
> 
> 
> 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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

> On Dec. 19, 2018, 11:33 p.m., Morio Ramdenbourg wrote:
> > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
> > Lines 662 (patched)
> > 
> >
> > The second parameter in ConfVars corresponds to the Hive property name. 
> > 
> > E.g. metastore.client.filter.result -> 
> > hive.metastore.client.filter.result
> > 
> > So this should be: 
> > METASTORE_CLIENT_FILTER_RESULT("metastore.client.filter.result", 
> > "hive.metastore.client.filter.result" ...)
> 
> Na Li wrote:
> this is new field, and does not exist in hive configuration. Since we are 
> splitting HMS from Hive, does it make sense to add this new one in hive 
> configuration?
> 
> Morio Ramdenbourg wrote:
> Hmm, I'm not exactly sure - HMS is still part of the Hive code base.
> 
> Others who have been adding new properties to only HMS have been putting 
> Hive name, so I think it's safe. Someone should confirm this though.

I see both examples in MetastoreConf.java. I will talk with others to find out 
the rule.

1) hiveName is "hive." + varname
ADDED_JARS("metastore.added.jars.path", "hive.added.jars.path", "",
"This an internal parameter."),


2) hiveName is the same as varname
CATALOG_DEFAULT("metastore.catalog.default", "metastore.catalog.default", 
"hive",
"The default catalog to use when a catalog is not specified.  Default 
is 'hive' (the " +
"default catalog)."),


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/#review211446
---


On Dec. 21, 2018, 9:36 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69585/
> ---
> 
> (Updated Dec. 21, 2018, 9:36 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-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
>  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/4/
> 
> 
> 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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

> On Dec. 19, 2018, 11:33 p.m., Morio Ramdenbourg wrote:
> > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
> > Lines 816 (patched)
> > 
> >
> > Nit: This might benefit from some extra spacing or spreading them out 
> > into if statements. I had a difficult time reading the nested ternary 
> > operators (or maybe that's just me?)

I wrap it in a function. So it will be easier to read.


> On Dec. 19, 2018, 11:33 p.m., Morio Ramdenbourg wrote:
> > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
> > Lines 2458 (patched)
> > 
> >
> > This is the same method as the one on the HiveMetaStore. Would it be 
> > possible to move it to a common method under 
> > hive-standalone-metastore-common?
> 
> Na Li wrote:
> some variables used are different. such as isClientFilterEnabled vs 
> isServerFilterEnabled and filterHook. If we make is generate function, those 
> need to be input. No sure how valuable to make that change.
> 
> Morio Ramdenbourg wrote:
> Ah, didn't notice those. If that's the case, I'm not sure either how 
> valuable the generic method would be. Could probably drop this recommendation

I decide to have a common method under hive-standalone-metastore-common


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/#review211446
---


On Dec. 21, 2018, 9:36 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69585/
> ---
> 
> (Updated Dec. 21, 2018, 9:36 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-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
>  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/4/
> 
> 
> 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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

> On Dec. 19, 2018, 11:33 p.m., Morio Ramdenbourg wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 4621 (patched)
> > 
> >
> > This is the same method as the one on the HiveMetaStoreClient. Would it 
> > be possible to move it to a common method under 
> > hive-standalone-metastore-common?

This function authorize access to the table of the partitions. Sergion is going 
to implement this when filtering partitions in SENTRY-2481. So I will remove 
this function in HMS client and server


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/#review211446
---


On Dec. 21, 2018, 9:36 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69585/
> ---
> 
> (Updated Dec. 21, 2018, 9:36 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-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
>  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/4/
> 
> 
> 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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Karthik Manamcheri via Review Board]

> On Dec. 20, 2018, 2:46 p.m., Sergio Pena wrote:
> > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
> > Lines 220-221 (patched)
> > 
> >
> > 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.
> 
> Na Li wrote:
> removing the filter hooks is more like a workaround. It is more 
> straightforward to keep the hook configuration, and have specific 
> configuration to enable/disable filtering at HMS client and server.

If we remote the filter hooks we'd be removing it from both server and client. 
What we want is to be able to control one or the other. What if the server 
filtering causes a huge performance degradation for a particular use case? In 
that case I'd want to be able to turn off server side filtering only. That's 
why we need these two configs.


- Karthik


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/#review211465
---


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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Morio Ramdenbourg via Review Board]

> On Dec. 19, 2018, 11:33 p.m., Morio Ramdenbourg wrote:
> > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
> > Lines 2458 (patched)
> > 
> >
> > This is the same method as the one on the HiveMetaStore. Would it be 
> > possible to move it to a common method under 
> > hive-standalone-metastore-common?
> 
> Na Li wrote:
> some variables used are different. such as isClientFilterEnabled vs 
> isServerFilterEnabled and filterHook. If we make is generate function, those 
> need to be input. No sure how valuable to make that change.

Ah, didn't notice those. If that's the case, I'm not sure either how valuable 
the generic method would be. Could probably drop this recommendation


> On Dec. 19, 2018, 11:33 p.m., Morio Ramdenbourg wrote:
> > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
> > Lines 662 (patched)
> > 
> >
> > The second parameter in ConfVars corresponds to the Hive property name. 
> > 
> > E.g. metastore.client.filter.result -> 
> > hive.metastore.client.filter.result
> > 
> > So this should be: 
> > METASTORE_CLIENT_FILTER_RESULT("metastore.client.filter.result", 
> > "hive.metastore.client.filter.result" ...)
> 
> Na Li wrote:
> this is new field, and does not exist in hive configuration. Since we are 
> splitting HMS from Hive, does it make sense to add this new one in hive 
> configuration?

Hmm, I'm not exactly sure - HMS is still part of the Hive code base.

Others who have been adding new properties to only HMS have been putting Hive 
name, so I think it's safe. Someone should confirm this though.


- Morio


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/#review211446
---


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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

> On Dec. 20, 2018, 2:46 p.m., Sergio Pena wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 2934 (patched)
> > 
> >
> > What is this extra space?

just make it easier to read. I will remove it.


> On Dec. 20, 2018, 2:46 p.m., Sergio Pena wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 2910-2911 (original), 2943-2946 (patched)
> > 
> >
> > Why is this change needed? I don't see anything new except splitting it 
> > in two lines. This will stay in the git history.

I will revert the change


> On Dec. 20, 2018, 2:46 p.m., Sergio Pena wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Line 2931 (original), 2966-2970 (patched)
> > 
> >
> > Notice the firePreEvent before the filter. If we use that for 
> > authorization checks, then the filter is not required here.

I will remove the call for filterhook. But Sentry needs to implement 
authorization in PreEvent listener for read operation for HMS


> On Dec. 20, 2018, 2:46 p.m., Sergio Pena wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 3050-3053 (patched)
> > 
> >
> > Same question, why splitting the lines where if this patch doesn't need 
> > it?

will remove it


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/#review211465
---


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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

> On Dec. 20, 2018, 2:46 p.m., Sergio Pena wrote:
> > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
> > Lines 220-221 (patched)
> > 
> >
> > 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.

removing the filter hooks is more like a workaround. It is more straightforward 
to keep the hook configuration, and have specific configuration to 
enable/disable filtering at HMS client and server.


> On Dec. 20, 2018, 2:46 p.m., Sergio Pena wrote:
> > standalone-metastore/metastore-server/pom.xml
> > Lines 242-246 (patched)
> > 
> >
> > Why is this dep necessary? I wonder if this conflicts with the 
> > standalone solution where all the Hive dependences were removed.

It is not needed. I have removed it.


> On Dec. 20, 2018, 2:46 p.m., Sergio Pena wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 1405 (patched)
> > 
> >
> > 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.

The PreReadEvent calls sentry MetastoreAuthzBindingBase for authorization. As 
you can see in
https://github.com/apache/sentry/blob/master/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java#L169,
 it does not do anything on reading metadata. Therefore, we have to use filter 
to prevent user from reading metadata it does not have privileges. 

I can only remove the filter for get_database() or get_table() if we change 
sentry's implementation for that file.


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/#review211465
---


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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

> On Dec. 19, 2018, 11:33 p.m., Morio Ramdenbourg wrote:
> > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
> > Lines 2458 (patched)
> > 
> >
> > This is the same method as the one on the HiveMetaStore. Would it be 
> > possible to move it to a common method under 
> > hive-standalone-metastore-common?

some variables used are different. such as isClientFilterEnabled vs 
isServerFilterEnabled and filterHook. If we make is generate function, those 
need to be input. No sure how valuable to make that change.


> On Dec. 19, 2018, 11:33 p.m., Morio Ramdenbourg wrote:
> > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
> > Lines 662 (patched)
> > 
> >
> > The second parameter in ConfVars corresponds to the Hive property name. 
> > 
> > E.g. metastore.client.filter.result -> 
> > hive.metastore.client.filter.result
> > 
> > So this should be: 
> > METASTORE_CLIENT_FILTER_RESULT("metastore.client.filter.result", 
> > "hive.metastore.client.filter.result" ...)

this is new field, and does not exist in hive configuration. Since we are 
splitting HMS from Hive, does it make sense to add this new one in hive 
configuration?


> On Dec. 19, 2018, 11:33 p.m., Morio Ramdenbourg wrote:
> > standalone-metastore/metastore-server/pom.xml
> > Lines 242 (patched)
> > 
> >
> > Nit: Won't this add a Hive dependency to the standalone metastore 
> > server? 
> > 
> > May need to modify this comment here: 
> > https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/pom.xml#L158

good catch. I have removed this dependency. It is not needed.


> On Dec. 19, 2018, 11:33 p.m., Morio Ramdenbourg wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 78 (patched)
> > 
> >
> > I don't think this is being used anywhere, could probably remove

Yes. I removed it.


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/#review211446
---


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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Sergio Pena via Review Board]

---
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)


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)


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)


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)


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)


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)


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)


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)


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)


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
> 
> 
> 

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Morio Ramdenbourg via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/#review211446
---




standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
Lines 816 (patched)


Nit: This might benefit from some extra spacing or spreading them out into 
if statements. I had a difficult time reading the nested ternary operators (or 
maybe that's just me?)



standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
Lines 2458 (patched)


This is the same method as the one on the HiveMetaStore. Would it be 
possible to move it to a common method under hive-standalone-metastore-common?



standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
Lines 662 (patched)


The second parameter in ConfVars corresponds to the Hive property name. 

E.g. metastore.client.filter.result -> hive.metastore.client.filter.result

So this should be: 
METASTORE_CLIENT_FILTER_RESULT("metastore.client.filter.result", 
"hive.metastore.client.filter.result" ...)



standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
Lines 663 (patched)


I think it'd be useful to add what the default value will be to the 
description. So add "Defaults to true".



standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
Lines 664 (patched)


Same here - second value should be the Hive property name.



standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
Lines 665 (patched)


Same here - add default value to the description



standalone-metastore/metastore-server/pom.xml
Lines 242 (patched)


Nit: Won't this add a Hive dependency to the standalone metastore server? 

May need to modify this comment here: 
https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/pom.xml#L158



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 78 (patched)


I don't think this is being used anywhere, could probably remove



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 4621 (patched)


This is the same method as the one on the HiveMetaStoreClient. Would it be 
possible to move it to a common method under hive-standalone-metastore-common?


- Morio Ramdenbourg


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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

---
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 (updated)
-

  
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/

Changes: https://reviews.apache.org/r/69585/diff/2-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

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

> On Dec. 19, 2018, 4:43 p.m., Karthik Manamcheri wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 5235 (patched)
> > 
> >
> > Can we make a helper fuction called "filterTablesIfEnabled"?

Done.


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/#review211436
---


On Dec. 19, 2018, 5:07 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, 5:07 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/2/
> 
> 
> 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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

> On Dec. 19, 2018, 4:43 p.m., Karthik Manamcheri wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 2956 (patched)
> > 
> >
> > All of this logic of endFunction seems to be inside getTableInternal. 
> > Why duplicate it here?

I wanted to show the filtering overhead. That is why. Now I move the filter 
code in getTableInternal(), and remove the added code in get_table()


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/#review211436
---


On Dec. 19, 2018, 5:07 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, 5:07 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/2/
> 
> 
> 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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Karthik Manamcheri via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/#review211436
---




standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
Lines 662 (patched)


We should rename this to "metastore.client.filter.enabled" and 
"metastore.server.filter.enabled"



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 2948 (patched)


getTableInternal also fires the PreReadTableEvent. I don't think we will 
need to fire this twice.



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 2956 (patched)


All of this logic of endFunction seems to be inside getTableInternal. Why 
duplicate it here?



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 5235 (patched)


Can we make a helper fuction called "filterTablesIfEnabled"?


- Karthik Manamcheri


On Dec. 19, 2018, 6:57 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69585/
> ---
> 
> (Updated Dec. 19, 2018, 6:57 a.m.)
> 
> 
> Review request for hive, 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/2/
> 
> 
> 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
> 
>

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/
---

(Updated Dec. 19, 2018, 6:57 a.m.)


Review request for hive, 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/2/


Testing (updated)
---

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

Re: Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/
---

(Updated Dec. 19, 2018, 6:56 a.m.)


Review request for hive, 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 (updated)
-

  
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/2/

Changes: https://reviews.apache.org/r/69585/diff/1-2/


Testing
---

Existing unit tests passed. 
Will add new unit tests for filtering at HMS server in next update of this jira
Will add code to enabled/disable filtering at HMS client based on configuration 
in next update of this jira


Thanks,

Na Li

Review Request 69585: HIVE-20776: Run HMS filterHooks on server-side in addition to client-side

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69585/
---

Review request for hive, 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/conf/MetastoreConf.java
 fb0b2fe 
  standalone-metastore/metastore-server/pom.xml 895abfc 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
 0a1b96d 


Diff: https://reviews.apache.org/r/69585/diff/1/


Testing
---

Existing unit tests passed. 
Will add new unit tests for filtering at HMS server in next update of this jira
Will add code to enabled/disable filtering at HMS client based on configuration 
in next update of this jira


Thanks,

Na Li