Re: Review Request 66336: SENTRY-2190: Have verbose debug logs in CounterWait class

[Na Li via Review Board]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/CounterWait.java
Lines 191 (patched)


can you put the currentId in the log message? That would be helpful



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/CounterWait.java
Lines 203 (patched)


Can you put the currentId in log message?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/CounterWait.java
Lines 214 (patched)


can you put currentId in log message to help correlate the events?


- Na Li


On March 29, 2018, 5:25 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66336/
> ---
> 
> (Updated March 29, 2018, 5:25 p.m.)
> 
> 
> Review request for sentry and Na Li.
> 
> 
> Bugs: SENTRY-2190
> https://issues.apache.org/jira/browse/SENTRY-2190
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Debug logging in CounterWait is not good enough to actually an issue around 
> this area. It's a good idea to extend the current logging.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/CounterWait.java
>  d8c82970b56b1599a07f0e26edab8ed3d59b9948 
> 
> 
> Diff: https://reviews.apache.org/r/66336/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 66360: SENTRY-2192: supress date value in @Generated annotation generated by thrift

[Na Li via Review Board]

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



It would be great if you can find out why some files the variable names are 
changed.

- Na Li


On March 29, 2018, 1:16 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66360/
> ---
> 
> (Updated March 29, 2018, 1:16 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Na Li, Steve Moist, and Sergio 
> Pena.
> 
> 
> Bugs: SENTRY-2192
> https://issues.apache.org/jira/browse/SENTRY-2192
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> With the current thrift options used, @Generated annotation adds date which 
> kind of updates all the thrift generated files when --thriftif option used.
> 
> When someone makes some changes to any of the thrift definitions and tries to 
> generate the source all the auto generated files get updated.
> 
> This can be avoided by suppressing date in the @generated annotation.
> 
> 
> Diffs
> -
> 
>   sentry-hdfs/sentry-hdfs-common/pom.xml 
> 5c6c96c46ee384ba5981611333741fde3bd59e10 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/SentryHDFSService.java
>  ac1fa6af17239e23557614e7149c7cc911e28e12 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TAuthzUpdateRequest.java
>  62fd0bb2426db427241a5f805c9e250b306b4947 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TAuthzUpdateResponse.java
>  68aec801ae7f38f8cbbe8549f5d9df79b609b114 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathChanges.java
>  8d3297ffe6f2de4dc3dc82f82a70b5ad2a235911 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathEntry.java
>  210d2191c38603d4a951a590eee85c8fccdde124 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathsDump.java
>  df5b7b10656102c6471885cfea73b0ed8dd67dd7 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathsUpdate.java
>  9fd3924f138696707df4ab1bb5c994005b9bd1f4 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPermissionsUpdate.java
>  e8c09322ac6f8b8210bc4dd8869ad3b0353b63d9 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegeChanges.java
>  0472f33ec49652ef402bcb332321fb2803b923d9 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TRoleChanges.java
>  1a502080896b2b85e9bf6ae4780b4a2db35473f7 
>   sentry-provider/sentry-provider-db/pom.xml 
> 4751549bcc40d60ca8f5400dd371761bc6fbf2d5 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyService.java
>  ae4db6a06c72a93e62bc5a3c2a17776bcb7d 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleAddGroupsRequest.java
>  0b62dae0588859bceea54b8f4b49689561a7a4f9 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleAddGroupsResponse.java
>  29e5baf8d3ecd67ae655f513eaba5a1a2bc14b9a 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleDeleteGroupsRequest.java
>  42293b7bbda879d87f69721cfc3f4fd533055ae2 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleDeleteGroupsResponse.java
>  a3a660cc9d53411588c5a681b84a75a35ebc3f96 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleGrantPrivilegeRequest.java
>  49cfe4b0057721839c442353b6441013d5d6438c 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleGrantPrivilegeResponse.java
>  6890337d6262500283086abf54e19ff1450796f8 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleRevokePrivilegeRequest.java
>  91793fee0f3a96a2fdec0cfecac325c65c08c999 
>   
> 

Re: Review Request 66373: SENTRY-2194: Upgrade Sentry hadoop-version dependency to 2.7.5 to take advantage of security vulnerability fix

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On March 30, 2018, 7:05 a.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66373/
> ---
> 
> (Updated March 30, 2018, 7:05 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar 
> kalvagadda, Na Li, Steve Moist, Sergio Pena, Vadim Spector, and Xinran Tinney.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> MapReduce team had discovered a security vulnerability in parsing Map 
> Reduce-Job History Server configuration. That private files owned by a user 
> running the cluster, could be exposed. This has been applied to versions - 
> 2.7.5, 2.8.3, 2.9.0, or 3.0.0. Since Sentry uses hadoop-common Configuration 
> class to parse xml files, this change can be accommodated by our produce as 
> well. Sentry upstream is currently using 2.7.2 hadoop.version and we should 
> bump up this version to 2.7.5 to take advantage of this feature.
> 
> The hadoop change involves adding a new boolean attribute restrictParser. 
> Setting restrictParser to true will
> 
> Limit XML parsing to conform with feature 
> "http://apache.org/xml/features/disallow-doctype-decl;
> This is a security feature explained here - 
> https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Prevention_Cheat_Sheet
> boolean restrictSystemProps is set to true
> Will prevent system properties from being read
> set XML inclusion (XInclude) to false
> prevent merging of xml documents
> With this change on hadoop side, only default resources, and hadoop-site.xml 
> have this feature turned off, so they will be read without restricted 
> parsing. Sentry is not listed as a default resource and would therefore have 
> to explicitly have this property set to true.
> 
> 
> Diffs
> -
> 
>   pom.xml 61e0f9700 
>   
> sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/authz/HBaseIndexerAuthzBinding.java
>  d919fe702 
>   
> sentry-binding/sentry-binding-hbase-indexer/src/main/java/org/apache/sentry/binding/hbaseindexer/conf/HBaseIndexerAuthzConf.java
>  cfbd37bf1 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  8d5286cd3 
>   
> sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/conf/KafkaAuthConf.java
>  6ca621022 
>   
> sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/conf/SolrAuthzConf.java
>  0883e70fe 
>   
> sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/conf/SqoopAuthConf.java
>  7836871f6 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryINodeAttributesProvider.java
>  cf96df47b 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/HadoopGroupMappingService.java
>  00b5cf608 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/PermissionsMigrationToolCommon.java
>  ed28b735c 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryConfigToolIndexer.java
>  c2341d322 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryConfigToolSolr.java
>  5649f43fa 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellGeneric.java
>  907e1462c 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java
>  729a51865 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  bf5d85b03 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java
>  b234c85c5 
>   sentry-tools/src/main/java/org/apache/sentry/shell/SentryCli.java 8b68d0d06 
> 
> 
> Diff: https://reviews.apache.org/r/66373/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 66376: SENTRY-2193: Synchronize thrift definition with the generated sources

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On March 30, 2018, 4:33 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66376/
> ---
> 
> (Updated March 30, 2018, 4:33 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov and Na Li.
> 
> 
> Bugs: SENTRY-2193
> https://issues.apache.org/jira/browse/SENTRY-2193
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Current thrift definition is not in-line with the the generated source. I 
> suspect that some of the commits that performed thrift changes did not commit 
> some of the files that are updated.
> 
> This would have been the case as the all the generated files were updated 
> because of the date change. Committers have an habbit of reverting the 
> changes. In that process some one would have reverted some of the changes 
> that are made.
> 
> Intent of this change is Synchronize thrift definition with the generated 
> sources. Once this we synchronize them and run below command only changes one 
> should be seeing are the date changes.
> 
> mvn clean install -Pthriftif -DskipTests -Dthrift.home=/usr/local/
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/SentryHDFSService.java
>  ac1fa6af17239e23557614e7149c7cc911e28e12 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TAuthzUpdateRequest.java
>  62fd0bb2426db427241a5f805c9e250b306b4947 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TAuthzUpdateResponse.java
>  68aec801ae7f38f8cbbe8549f5d9df79b609b114 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathChanges.java
>  8d3297ffe6f2de4dc3dc82f82a70b5ad2a235911 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathEntry.java
>  210d2191c38603d4a951a590eee85c8fccdde124 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathsDump.java
>  df5b7b10656102c6471885cfea73b0ed8dd67dd7 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathsUpdate.java
>  9fd3924f138696707df4ab1bb5c994005b9bd1f4 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPermissionsUpdate.java
>  e8c09322ac6f8b8210bc4dd8869ad3b0353b63d9 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegeChanges.java
>  0472f33ec49652ef402bcb332321fb2803b923d9 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TRoleChanges.java
>  1a502080896b2b85e9bf6ae4780b4a2db35473f7 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyService.java
>  ae4db6a06c72a93e62bc5a3c2a17776bcb7d 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleAddGroupsRequest.java
>  0b62dae0588859bceea54b8f4b49689561a7a4f9 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleAddGroupsResponse.java
>  29e5baf8d3ecd67ae655f513eaba5a1a2bc14b9a 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleDeleteGroupsRequest.java
>  42293b7bbda879d87f69721cfc3f4fd533055ae2 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleDeleteGroupsResponse.java
>  a3a660cc9d53411588c5a681b84a75a35ebc3f96 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleGrantPrivilegeRequest.java
>  49cfe4b0057721839c442353b6441013d5d6438c 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleGrantPrivilegeResponse.java
>  6890337d6262500283086abf54e19ff1450796f8 
>   
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/generic/service/thrift/TAlterSentryRoleRevokePrivilegeRequest.java
>  91793fee0f3a96a2fdec0cfecac325c65c08c999 
>   
> 

Re: Review Request 66065: SENTRY-2160: Add owner in create table notification event

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On March 14, 2018, 2:36 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66065/
> ---
> 
> (Updated March 14, 2018, 2:36 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2160
> https://issues.apache.org/jira/browse/SENTRY-2160
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When creating notification event in SentryJSONCreateTableMessage owner 
> information from the Table instance should be updated properly.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONCreateTableMessage.java
>  61d94819816ab281207d14bf68791325af635c11 
>   
> sentry-binding/sentry-binding-hive-follower/src/test/java/org/apache/sentry/binding/metastore/messaging/json/TestSentryJSONCreateTableMessage.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  61e3f06159ec96f345145cb2b3ed6aac2abbe0ae 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestNotificationProcessor.java
>  923fafff128b17981b62f5dcf1e872ab27434b5f 
> 
> 
> Diff: https://reviews.apache.org/r/66065/diff/1/
> 
> 
> Testing
> ---
> 
> Added new tests an also made sure that all the tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 66066: SENTRY-2181: Add owner privileges to create database notifications

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On March 14, 2018, 2:43 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66066/
> ---
> 
> (Updated March 14, 2018, 2:43 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2181
> https://issues.apache.org/jira/browse/SENTRY-2181
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch makes changes so that owner privileges are updated in the 
> SentryJSONCreateDatabaseMessage on receiving create database event.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONCreateDatabaseMessage.java
>  a519fd7bad2b99ae57e46afdccdf325a37def0cd 
>   
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageFactory.java
>  0d0c73fa989dab32820cef52281e261f19fc54e8 
>   
> sentry-binding/sentry-binding-hive-follower/src/test/java/org/apache/sentry/binding/metastore/messaging/json/TestSentryJSONCreateDatabaseMessage.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java
>  c6be80d71981cf75d8e76a063b6cda0d75c65fb4 
> 
> 
> Diff: https://reviews.apache.org/r/66066/diff/1/
> 
> 
> Testing
> ---
> 
> added new test and also made made sure that all the tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 66066: SENTRY-2181: Add owner privileges to create database notifications

[Na Li via Review Board]

> On March 14, 2018, 3:56 p.m., Na Li wrote:
> > what jira will you add code to handle db events processing relate to owner 
> > privileges?
> 
> kalyan kumar kalvagadda wrote:
> SENTRY-2153 will add code to update the cache.
> There should another jira created to add inplcit privileges.
> 
> Na Li wrote:
> Is there a jira to add owner privileges when owner privileges change in 
> alter database event?
> Is there a jira to get current owner privileges of databases.
> 
> kalyan kumar kalvagadda wrote:
> There is an issue with alter database event. hive 2.3.2 doesn't support 
> alter database event.
> I have created to SENTRY-2182 to extend the SentryJSONMessageFactory but 
> it can not be done until we have this functionality in hive version that 
> sentry uses.
> 
> Can you elaborte, "Is there a jira to get current owner privileges of 
> databases.". In either way this it out of scope of this jira.

I meant when sentry server starts and is leader, it should get current owner 
privileges of existing databases. Just processing events for creating database 
and alter database is not enough. SENTRY-2153 includes this. It originally 
includes the effort for this jira and SENTRY-2182. Can you update the 
description for sentry-2153 since you splited the effort into 3 jira?


- Na


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


On March 14, 2018, 2:43 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66066/
> ---
> 
> (Updated March 14, 2018, 2:43 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2181
> https://issues.apache.org/jira/browse/SENTRY-2181
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch makes changes so that owner privileges are updated in the 
> SentryJSONCreateDatabaseMessage on receiving create database event.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONCreateDatabaseMessage.java
>  a519fd7bad2b99ae57e46afdccdf325a37def0cd 
>   
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONMessageFactory.java
>  0d0c73fa989dab32820cef52281e261f19fc54e8 
>   
> sentry-binding/sentry-binding-hive-follower/src/test/java/org/apache/sentry/binding/metastore/messaging/json/TestSentryJSONCreateDatabaseMessage.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java
>  c6be80d71981cf75d8e76a063b6cda0d75c65fb4 
> 
> 
> Diff: https://reviews.apache.org/r/66066/diff/1/
> 
> 
> Testing
> ---
> 
> added new test and also made made sure that all the tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Review Request 66146: SENTRY-2184: Performance Issue: MPath is queried for each MAuthzPathsMapping in full snapshot

[Na Li via Review Board]

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

Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar 
kalvagadda, and Sergio Pena.


Repository: sentry


Description
---

Configure DataNucleus to fetch MPath in bulk to improve performance


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
 d883c51 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 4521ad4 


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


Testing
---

Run unti tests.
Check log message that veries that the query for MPath is issued in bullk.


Thanks,

Na Li

Re: Review Request 66146: SENTRY-2184: Performance Issue: MPath is queried for each MAuthzPathsMapping in full snapshot

[Na Li via Review Board]

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

(Updated March 20, 2018, 11:54 p.m.)


Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar 
kalvagadda, and Sergio Pena.


Repository: sentry


Description
---

Configure DataNucleus to fetch MPath in bulk to improve performance


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
 d883c51 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 4521ad4 


Diff: https://reviews.apache.org/r/66146/diff/2/

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


Testing
---

Run unti tests.
Check log message that veries that the query for MPath is issued in bullk.


Thanks,

Na Li

Re: Review Request 66146: SENTRY-2184: Performance Issue: MPath is queried for each MAuthzPathsMapping in full snapshot

[Na Li via Review Board]

> On March 20, 2018, 8:06 p.m., kalyan kumar kalvagadda wrote:
> > Lina, Code looks good but we need to run some performance tests with some 
> > huge data to actually see the results.
> > Simple unit test may not be good enough.

The performance test code and result is posted in jira sentry-2184


> On March 20, 2018, 8:06 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 2616 (patched)
> > 
> >
> > Isn't using fp.setGroup(FetchPlan.DEFAULT) a better option?

This line is useless. Removing "pm.getFetchPlan().setMaxFetchDepth(2);" does 
not make difference. As long as the fetch groups are added to the pm, the MPath 
is fetched in bulk.


- Na


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


On March 20, 2018, 11:54 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66146/
> ---
> 
> (Updated March 20, 2018, 11:54 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Configure DataNucleus to fetch MPath in bulk to improve performance
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  d883c51 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  4521ad4 
> 
> 
> Diff: https://reviews.apache.org/r/66146/diff/2/
> 
> 
> Testing
> ---
> 
> Run unti tests.
> Check log message that veries that the query for MPath is issued in bullk.
> 
> 
> Thanks,
> 
> Na Li
> 
>

Review Request 66265: SENTRY-2155: Update JDO to grant privileges to user

[Na Li via Review Board]

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

Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar 
kalvagadda, and Sergio Pena.


Repository: sentry


Description
---

Update application code and package.jdo to add mapping between user and 
privileges


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java
 0e8fb06 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
 73fa4ff 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUser.java
 f468a46 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
 9ce9cae 


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


Testing
---

build succeeds. Need to do more testing. It should be committed after 
sentry-2154 is committed. Before finishing SENTRY-2156, we cannot do much 
meaningful tests


Thanks,

Na Li

Review Request 66263: SENTRY-2154: Update schema to grant privileges to user.

[Na Li via Review Board]

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

Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar 
kalvagadda, and Sergio Pena.


Repository: sentry


Description
---

update mysql related scripts to 
1) dd new DB table SENTRY_USER_DB_PRIVILEGE_MAP and 
SENTRY_USER_GM_PRIVILEGE_MAP to support grant user to privileges
2)  a flag is added in privilege table to indicate the privilege is created by 
user, or created by sentry implicitly. I will make change in application code 
so User can view the implicit privileges, but cannot change it directly

The current change only for MySql. Once we finalize the schema, I will add more 
scripts for other DBs


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/resources/010-SENTRY-2154.mysql.sql 
PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-2.1.0.sql 
PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-mysql-2.0.0-to-2.1.0.sql
 PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.mysql 
770e1b5 


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


Testing
---

Run the script and update script, and the tables are created and updated 
successfully.


Thanks,

Na Li

Re: Review Request 66146: SENTRY-2184: Performance Issue: MPath is queried for each MAuthzPathsMapping in full snapshot

[Na Li via Review Board]

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

(Updated March 21, 2018, 4:52 p.m.)


Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar 
kalvagadda, and Sergio Pena.


Repository: sentry


Description
---

Configure DataNucleus to fetch MPath in bulk to improve performance


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
 d883c51 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 4521ad4 


Diff: https://reviews.apache.org/r/66146/diff/3/

Changes: https://reviews.apache.org/r/66146/diff/2-3/


Testing
---

Run unti tests.
Check log message that veries that the query for MPath is issued in bullk.


Thanks,

Na Li

Re: Review Request 65867: SENTRY-2147 - Fix Javadoc for SentryHiveAuthorizerFactory

[Na Li via Review Board]

> On March 2, 2018, 3:43 p.m., Na Li wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryHiveAuthorizerFactory.java
> > Line 34 (original), 34 (patched)
> > 
> >
> > can you use "the hive-site.xml at hive server 2" instead of 
> > "hive-site.xml"? hive-site.xml can be at both hive server 2, HMS, or at 
> > sentry.
> 
> Colm O hEigeartaigh wrote:
> It's hiveserver2-site.xml though not hive-site.xml?

In our unit tests and e2e tests, the hive configuration files are always called 
hive-site.xml. Would it be confusing for someone to see hiveserver2-site.xml? I 
searched the code, there is no other places refer such file name.


- Na


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


On March 1, 2018, 12:32 p.m., Colm O hEigeartaigh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65867/
> ---
> 
> (Updated March 1, 2018, 12:32 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-2147
> https://issues.apache.org/jira/browse/SENTRY-2147
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The Javadoc for SentryHiveAuthorizerFactory incorrectly states that it should 
> be configured as follows:
> 
>  
>hive.security.authorization.enable
>   
> org.apache.sentry.binding.hive.authz.SentryHiveAuthorizerFactory
> 
> 
> Instead it should be "hive.security.authorization.manager".
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryHiveAuthorizerFactory.java
>  db8e4288 
> 
> 
> Diff: https://reviews.apache.org/r/65867/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Colm O hEigeartaigh
> 
>

Re: Review Request 65867: SENTRY-2147 - Fix Javadoc for SentryHiveAuthorizerFactory

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On March 1, 2018, 12:32 p.m., Colm O hEigeartaigh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65867/
> ---
> 
> (Updated March 1, 2018, 12:32 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-2147
> https://issues.apache.org/jira/browse/SENTRY-2147
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The Javadoc for SentryHiveAuthorizerFactory incorrectly states that it should 
> be configured as follows:
> 
>  
>hive.security.authorization.enable
>   
> org.apache.sentry.binding.hive.authz.SentryHiveAuthorizerFactory
> 
> 
> Instead it should be "hive.security.authorization.manager".
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryHiveAuthorizerFactory.java
>  db8e4288 
> 
> 
> Diff: https://reviews.apache.org/r/65867/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Colm O hEigeartaigh
> 
>

Re: Review Request 65867: SENTRY-2147 - Fix Javadoc for SentryHiveAuthorizerFactory

[Na Li via Review Board]

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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryHiveAuthorizerFactory.java
Line 34 (original), 34 (patched)


can you use "the hive-site.xml at hive server 2" instead of 
"hive-site.xml"? hive-site.xml can be at both hive server 2, HMS, or at sentry.


- Na Li


On March 1, 2018, 12:32 p.m., Colm O hEigeartaigh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65867/
> ---
> 
> (Updated March 1, 2018, 12:32 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-2147
> https://issues.apache.org/jira/browse/SENTRY-2147
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The Javadoc for SentryHiveAuthorizerFactory incorrectly states that it should 
> be configured as follows:
> 
>  
>hive.security.authorization.enable
>   
> org.apache.sentry.binding.hive.authz.SentryHiveAuthorizerFactory
> 
> 
> Instead it should be "hive.security.authorization.manager".
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryHiveAuthorizerFactory.java
>  db8e4288 
> 
> 
> Diff: https://reviews.apache.org/r/65867/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Colm O hEigeartaigh
> 
>

Re: Review Request 66764: SENTRY-2210: AUTHZ_PATH should have index on the foreign key AUTHZ_OBJ_ID

[Na Li via Review Board]

> On April 25, 2018, 8:21 p.m., Sergio Pena wrote:
> > Lina, would it be better to create another patch to fix the 2.1.0 versions? 
> > So we can have this patch to add the index only instead of several new 
> > files, what do you think?

Sergio, I don't want to take the trouble to fix the 2.1.0 versions. However, 
2.0 is already released, any schema change on top of that has to mark it as for 
version 2.1.0. Is that correct?


- Na


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


On April 23, 2018, 10:56 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66764/
> ---
> 
> (Updated April 23, 2018, 10:56 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> add index for the foreign key AUTHZ_OBJ_ID on table AUTHZ_PATH
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java
>  f1a5b10 
>   
> sentry-provider/sentry-provider-db/src/main/resources/010-SENTRY-2210.derby.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/010-SENTRY-2210.mysql.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/010-SENTRY-2210.oracle.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/010-SENTRY-2210.postgres.sql
>  PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-2.1.0.sql 
> PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-2.1.0.sql 
> PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-2.1.0.sql 
> PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-2.1.0.sql 
> PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-2.1.0.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-2.0.0-to-2.1.0.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-derby-2.0.0-to-2.1.0.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-mysql-2.0.0-to-2.1.0.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-oracle-2.0.0-to-2.1.0.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-postgres-2.0.0-to-2.1.0.sql
>  PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.db2 
> 5ae9349 
>   sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.derby 
> 770e1b5 
>   sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.mysql 
> 770e1b5 
>   sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.oracle 
> 770e1b5 
>   
> sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.postgres 
> 770e1b5 
> 
> 
> Diff: https://reviews.apache.org/r/66764/diff/3/
> 
> 
> Testing
> ---
> 
> run mysql script to verify it works on mysql
> SentrySchemaTool upgrade/install tests
> 
> 
> Thanks,
> 
> Na Li
> 
>

Review Request 66826: SENTRY-2213: Increase schema version from 2.0.0 to 2.1.0

[Na Li via Review Board]

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

Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar 
kalvagadda, and Sergio Pena.


Repository: sentry


Description
---

Increase the schema version from 2.0.0 to 2.1.0


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java
 f1a5b10 
  sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-2.1.0.sql 
PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-2.1.0.sql 
PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-2.1.0.sql 
PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-2.1.0.sql 
PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-2.1.0.sql 
PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-2.0.0-to-2.1.0.sql
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-derby-2.0.0-to-2.1.0.sql
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-mysql-2.0.0-to-2.1.0.sql
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-oracle-2.0.0-to-2.1.0.sql
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-postgres-2.0.0-to-2.1.0.sql
 PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.db2 
5ae9349 
  sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.derby 
770e1b5 
  sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.mysql 
770e1b5 
  sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.oracle 
770e1b5 
  sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.postgres 
770e1b5 


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


Testing
---

TestSentrySchemaTool


Thanks,

Na Li

Review Request 67774: SENTRY-2289: Tests in TestHDFSIntegrationAdvanced fail from time to time

[Na Li via Review Board]

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

Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.


Bugs: sentry-2289
https://issues.apache.org/jira/browse/sentry-2289


Repository: sentry


Description
---

wait long enough before verify the result to make sure previous test cleanup 
does not remove new test setup. And new test wait enough time before verify 
result


Diffs
-

  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java
 c8fc019 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 100219b 


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


Testing
---

local run of TestHDFSIntegrationAdvanced succeed


Thanks,

Na Li

Re: Review Request 66336: SENTRY-2190: Have verbose debug logs in CounterWait class

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On March 30, 2018, 11:10 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66336/
> ---
> 
> (Updated March 30, 2018, 11:10 p.m.)
> 
> 
> Review request for sentry and Na Li.
> 
> 
> Bugs: SENTRY-2190
> https://issues.apache.org/jira/browse/SENTRY-2190
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Debug logging in CounterWait is not good enough to actually an issue around 
> this area. It's a good idea to extend the current logging.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/CounterWait.java
>  d8c82970b56b1599a07f0e26edab8ed3d59b9948 
> 
> 
> Diff: https://reviews.apache.org/r/66336/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67695: SENTRY-2277: Add to SentryStore testURI test case testing with multiple URI privileges

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On June 21, 2018, 9:18 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67695/
> ---
> 
> (Updated June 21, 2018, 9:18 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This is a follow up to SENTRY-2231 to add test cases to validate
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  51048bc29 
> 
> 
> Diff: https://reviews.apache.org/r/67695/diff/1/
> 
> 
> Testing
> ---
> 
> $  mvn -f sentry-service/sentry-service-server/pom.xml test 
> -Dtest=TestSentryStore
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68958: SENTRY-2419: Log where sentry stands in the process of persisting the snpashot

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3437 (patched)


if there are millions of partitions, will this take a long time?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3463 (patched)


why do you need to timestamp the message? logging will have timestamp.


- Na Li


On Oct. 12, 2018, 2 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68958/
> ---
> 
> (Updated Oct. 12, 2018, 2 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2419
> https://issues.apache.org/jira/browse/SENTRY-2419
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Process of persisting the snapshot might take longer based on the snapshot 
> size. It would be helpfull to to be able to understand where the sentry 
> sentry stands in the process of persisting.
> 
>  
> 
> Adding a Metric for this will be helpfull in debugging.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  d34340cd6 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  b6dca7aa8 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a736ca96 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  3a68eb6bf 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  059621a68 
> 
> 
> Diff: https://reviews.apache.org/r/68958/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68958: SENTRY-2419: Log where sentry stands in the process of persisting the snpashot

[Na Li via Review Board]

> On Oct. 12, 2018, 3:43 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3437 (patched)
> > 
> >
> > if there are millions of partitions, will this take a long time?

the overhead only depends on the number of authorization objects. So it should 
be ok.


- Na


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


On Oct. 12, 2018, 2 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68958/
> ---
> 
> (Updated Oct. 12, 2018, 2 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2419
> https://issues.apache.org/jira/browse/SENTRY-2419
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Process of persisting the snapshot might take longer based on the snapshot 
> size. It would be helpfull to to be able to understand where the sentry 
> sentry stands in the process of persisting.
> 
>  
> 
> Adding a Metric for this will be helpfull in debugging.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  d34340cd6 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  b6dca7aa8 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a736ca96 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  3a68eb6bf 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  059621a68 
> 
> 
> Diff: https://reviews.apache.org/r/68958/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68958: SENTRY-2419: Log where sentry stands in the process of persisting the snpashot

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Oct. 12, 2018, 2 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68958/
> ---
> 
> (Updated Oct. 12, 2018, 2 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2419
> https://issues.apache.org/jira/browse/SENTRY-2419
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Process of persisting the snapshot might take longer based on the snapshot 
> size. It would be helpfull to to be able to understand where the sentry 
> sentry stands in the process of persisting.
> 
>  
> 
> Adding a Metric for this will be helpfull in debugging.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  d34340cd6 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  b6dca7aa8 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a736ca96 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  3a68eb6bf 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  059621a68 
> 
> 
> Diff: https://reviews.apache.org/r/68958/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68823: SENTRY-2416: FullUpdateInitializer metrics are not reset for each new HMS snapshot

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateInitializer.java
Lines 261 (patched)


should you check if the number of db, tables and partitions are fetched?


- Na Li


On Sept. 24, 2018, 3:03 a.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68823/
> ---
> 
> (Updated Sept. 24, 2018, 3:03 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2416
> https://issues.apache.org/jira/browse/sentry-2416
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Resets the FullUpdateInitializer metrics counters to zero if another snapshot 
> process starts. This will help to track how many objects are being requested 
> from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  3e27d1bbee9bea9a60e7f7e012a367957610826c 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateInitializer.java
>  589acbed12855ff09309a04c9214f8daf87ea1de 
> 
> 
> Diff: https://reviews.apache.org/r/68823/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 69030: SENTRY-2427: Use Hadoop KerberosName class to derive shortName

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/GSSCallback.java
Lines 70 (patched)


should we have a unit test to show how it is used?


- Na Li


On Oct. 15, 2018, 6:42 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69030/
> ---
> 
> (Updated Oct. 15, 2018, 6:42 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2427
> https://issues.apache.org/jira/browse/SENTRY-2427
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry doesn't use auth to local group mapping hadoop configuration. We may 
> have a use case for cross realm users to have access to sentry service and in 
> which case Sentry needs to have access to those configurations. Switching to 
> using KerberosName will handle that case and other cases as well
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/GSSCallback.java
>  d2d85d3a2 
> 
> 
> Diff: https://reviews.apache.org/r/69030/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69030: SENTRY-2427: Use Hadoop KerberosName class to derive shortName

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestGSSCallback.java
Lines 52 (patched)


You never set HADOOP_SECURITY_AUTH_TO_LOCAL in conf. What is different from 
following code

KerberosName.setRules(defaultRule);

instead of 
String defaultRule = "DEFAULT";
String ruleString = conf.get(HADOOP_SECURITY_AUTH_TO_LOCAL, 
defaultRule);
KerberosName.setRules(ruleString);



sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestGSSCallback.java
Lines 70 (patched)


from this rule, it seems "us...@test.realm.com" should be allowed. What's 
the reason it is not valid?


- Na Li


On Oct. 15, 2018, 9:56 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69030/
> ---
> 
> (Updated Oct. 15, 2018, 9:56 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2427
> https://issues.apache.org/jira/browse/SENTRY-2427
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry doesn't use auth to local group mapping hadoop configuration. We may 
> have a use case for cross realm users to have access to sentry service and in 
> which case Sentry needs to have access to those configurations. Switching to 
> using KerberosName will handle that case and other cases as well
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/GSSCallback.java
>  d2d85d3a2 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestGSSCallback.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69030/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69030: SENTRY-2427: Use Hadoop KerberosName class to derive shortName

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Oct. 16, 2018, 4:17 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69030/
> ---
> 
> (Updated Oct. 16, 2018, 4:17 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2427
> https://issues.apache.org/jira/browse/SENTRY-2427
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry doesn't use auth to local group mapping hadoop configuration. We may 
> have a use case for cross realm users to have access to sentry service and in 
> which case Sentry needs to have access to those configurations. Switching to 
> using KerberosName will handle that case and other cases as well
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/GSSCallback.java
>  d2d85d3a2 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestGSSCallback.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69030/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68973: SENTRY-2305: Optimize time taken for persistence HMS snapshot by persisting in parallel

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3427 (patched)


can you log the value of authzPaths.size() and 
getMAuthzPathsMappingCountCore(pm, nextSnapshotID)? That could give us a sense 
how big a difference they are.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 5001 (patched)


why do you have this field? I don't see it is used in this class.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 5020 (patched)


can you log the value of snapshotID as well?


- Na Li


On Oct. 10, 2018, 11:04 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68973/
> ---
> 
> (Updated Oct. 10, 2018, 11:04 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2305
> https://issues.apache.org/jira/browse/SENTRY-2305
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> I have considered multiple options. Persisting in batches is not an option 
> with out changing the schema as the data nucleus does not persist row in 
> batches for tables which have foreign key on other tables.
> 
> I see that best option is to persist the paths in parallel. It gave good 
> results.
> 
> Solution Approach:
> 
> I have used a thread pool to persist the snapshot. Size of this thread pool 
> is configurable. Paths for each object database/table are submitted to this 
> thread pool. If for reason some of the paths are not pesisted, snapshot is 
> removed and exception is throw back.
> 
>   This patch along with SENTRY-2423 was 5 times faster when tested with below.
> 
>  
> 
> Object Type   Count
> Databases 209
> Tables2100
> Partitions24
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a736ca9604eb0bb182a159b5a2aed274275c16e 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  0d62941a7bd45e0d8a67b5d95fdb39a8801f5a26 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  4a9afe303672baff39be01d4f190034b2bfb75fe 
> 
> 
> Diff: https://reviews.apache.org/r/68973/diff/3/
> 
> 
> Testing
> ---
> 
> Added new test and also made sure that existing tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68968: SENTRY-2425: Add metric to track the time taken to update the owner privilege

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1515 (patched)


Sergio is moving owner privilege update from post event listener to 
notification processor. So your code here will be affected


- Na Li


On Oct. 18, 2018, 5:38 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68968/
> ---
> 
> (Updated Oct. 18, 2018, 5:38 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2425
> https://issues.apache.org/jira/browse/SENTRY-2425
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> New metric should be added to track the time taken by sentry server in 
> granting/updating owner privileges.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  214d78c5332d4113e440b76077bb7b2a6dae81d6 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  709434c388689b63d5db7d71cd6cc47952d647bf 
> 
> 
> Diff: https://reviews.apache.org/r/68968/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Review Request 69074: SENTRY-2429: Transfer database owner drops table owner

[Na Li via Review Board]

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

Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
Pena.


Bugs: sentry-2429
https://issues.apache.org/jira/browse/sentry-2429


Repository: sentry


Description
---

1) What is the problem?
When changing owner of a database, the owners of all tables belonging to this 
database are removed.

2) For example:
2.1) user_A is owner of DB1, user_B is owner of DB1.TABLE1, and user_D is owner 
of DB1.TABLE2.
2.2) admin with" ALL with grant option" changes the owner of DB1 from user_A to 
user_C through command "ALTER DATABASE DB1 SET OWNER USER user_C".
2.3) Now, user_C is owner of DB1, there is no owner on DB1.TABLE1, and no owner 
on DB1.TABLE2.. In other words, the owners of DB1.TABLE1 and DB1.TABLE2 are 
removed when changing owner of their database.


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 29f83a8 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
 880fa94 


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


Testing
---

add new test cases and verified the fix in e2e test


Thanks,

Na Li

Re: Review Request 68973: SENTRY-2305: Optimize time taken for persistence HMS snapshot by persisting in parallel

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 5106-5112 (patched)


Sergio,

Persisting full snapshot does not persist path change, so no changeID 
conflict


- Na Li


On Oct. 22, 2018, 2:42 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68973/
> ---
> 
> (Updated Oct. 22, 2018, 2:42 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2305
> https://issues.apache.org/jira/browse/SENTRY-2305
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> I have considered multiple options. Persisting in batches is not an option 
> with out changing the schema as the data nucleus does not persist row in 
> batches for tables which have foreign key on other tables.
> 
> I see that best option is to persist the paths in parallel. It gave good 
> results.
> 
> Solution Approach:
> 
> I have used a thread pool to persist the snapshot. Size of this thread pool 
> is configurable. Paths for each object database/table are submitted to this 
> thread pool. If for reason some of the paths are not pesisted, snapshot is 
> removed and exception is throw back.
> 
>   This patch along with SENTRY-2423 was 5 times faster when tested with below.
> 
>  
> 
> Object Type   Count
> Databases 209
> Tables2100
> Partitions24
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  b387a22e40b8958395e1c12af12272b89a778726 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  0d62941a7bd45e0d8a67b5d95fdb39a8801f5a26 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  4a9afe303672baff39be01d4f190034b2bfb75fe 
> 
> 
> Diff: https://reviews.apache.org/r/68973/diff/5/
> 
> 
> Testing
> ---
> 
> Added new test and also made sure that existing tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Review Request 69122: SENTRY-2432: The case of a username is ignored when determining object ownership

[Na Li via Review Board]

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

Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
Pena.


Bugs: sentry-2432
https://issues.apache.org/jira/browse/sentry-2432


Repository: sentry


Description
---

Keep user name case when saving into sentry DB and fetching privileges for that 
user.


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
 f29c455 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 b387a22 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 4a9afe3 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
 d3294f4 


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


Testing
---

add new test case to check authorization based on user name case


Thanks,

Na Li

Re: Review Request 69076: SENTRY-2301: Log where sentry stands in the snapshot fetching process, periodically

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
Lines 127 (patched)


We already have a timer for this purpose, name 
"org.apache.sentry.service.thrift.FullUpdateInitializer.snapshot"

It is defined in SentryHMSClient
  private final Timer updateTimer = SentryMetrics.getInstance()
  .getTimer(name(FullUpdateInitializer.class, SNAPSHOT));
  
It is called in 
SentryHMSClient.fetchFullUpdate()

Do we need another timer for the same purpose?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
Line 307 (original), 313 (patched)


You can change it to check if part.getSd() is null or not


- Na Li


On Oct. 18, 2018, 9:29 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69076/
> ---
> 
> (Updated Oct. 18, 2018, 9:29 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When sentry is fetching snapshot from HMS, it should log periodically on 
> where it stands in the snapshot process. This will help person debugging it 
> and help him understand the progress.
> 
>  
> 
> This is important as this process could take magnitude of minutes when the 
> HMS data is huge.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  214d78c53 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  3e27d1bbe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  4baeb6725 
> 
> 
> Diff: https://reviews.apache.org/r/69076/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68973: SENTRY-2305: Optimize time taken for persistence HMS snapshot by persisting in parallel

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 187 (patched)


should this variable be local to persistFullPathsImage, and be atomic as it 
is set by more than one thread.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 294 (patched)


When full snapshot fetch is done, should these counters be reset? 
Otherwise, they will keep on increasing, more than totalNumberOfObjectsToPersist


- Na Li


On Oct. 19, 2018, 4:53 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68973/
> ---
> 
> (Updated Oct. 19, 2018, 4:53 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2305
> https://issues.apache.org/jira/browse/SENTRY-2305
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> I have considered multiple options. Persisting in batches is not an option 
> with out changing the schema as the data nucleus does not persist row in 
> batches for tables which have foreign key on other tables.
> 
> I see that best option is to persist the paths in parallel. It gave good 
> results.
> 
> Solution Approach:
> 
> I have used a thread pool to persist the snapshot. Size of this thread pool 
> is configurable. Paths for each object database/table are submitted to this 
> thread pool. If for reason some of the paths are not pesisted, snapshot is 
> removed and exception is throw back.
> 
>   This patch along with SENTRY-2423 was 5 times faster when tested with below.
> 
>  
> 
> Object Type   Count
> Databases 209
> Tables2100
> Partitions24
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  b387a22e40b8958395e1c12af12272b89a778726 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  0d62941a7bd45e0d8a67b5d95fdb39a8801f5a26 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  4a9afe303672baff39be01d4f190034b2bfb75fe 
> 
> 
> Diff: https://reviews.apache.org/r/68973/diff/4/
> 
> 
> Testing
> ---
> 
> Added new test and also made sure that existing tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69122: SENTRY-2432: The case of a username is ignored when determining object ownership

[Na Li via Review Board]

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

(Updated Oct. 24, 2018, 4:14 p.m.)


Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
Pena.


Bugs: sentry-2432
https://issues.apache.org/jira/browse/sentry-2432


Repository: sentry


Description
---

Keep user name case when saving into sentry DB and fetching privileges for that 
user.


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
 f29c455 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 b387a22 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 4a9afe3 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
 d3294f4 


Diff: https://reviews.apache.org/r/69122/diff/2/

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


Testing
---

add new test case to check authorization based on user name case


Thanks,

Na Li

Re: Review Request 69122: SENTRY-2432: The case of a username is ignored when determining object ownership

[Na Li via Review Board]

> On Oct. 23, 2018, 4:38 p.m., kalyan kumar kalvagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 1118 (original), 1124 (patched)
> > 
> >
> > you can just call princilaNames.safeTrim()

I removed the trim in this function. In alterSentryRevokePrivilegeCore, I check 
if the variable is null and throw exception if so. Then just trim


- Na


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


On Oct. 24, 2018, 4:14 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69122/
> ---
> 
> (Updated Oct. 24, 2018, 4:14 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2432
> https://issues.apache.org/jira/browse/sentry-2432
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Keep user name case when saving into sentry DB and fetching privileges for 
> that user.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  f29c455 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  b387a22 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  4a9afe3 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
>  d3294f4 
> 
> 
> Diff: https://reviews.apache.org/r/69122/diff/2/
> 
> 
> Testing
> ---
> 
> add new test case to check authorization based on user name case
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69122: SENTRY-2432: The case of a username is ignored when determining object ownership

[Na Li via Review Board]

> On Oct. 23, 2018, 3:42 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 929 (original), 932 (patched)
> > 
> >
> > Should we add userName.trim() here?

It is trimmed in "MSentryUser getMSentryUserByName(final String userName, final 
boolean throwExceptionIfNotExist) "


> On Oct. 23, 2018, 3:42 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 984 (original), 990 (patched)
> > 
> >
> > Here?

what do you mean?


- Na


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


On Oct. 22, 2018, 10:04 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69122/
> ---
> 
> (Updated Oct. 22, 2018, 10:04 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2432
> https://issues.apache.org/jira/browse/sentry-2432
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Keep user name case when saving into sentry DB and fetching privileges for 
> that user.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  f29c455 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  b387a22 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  4a9afe3 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
>  d3294f4 
> 
> 
> Diff: https://reviews.apache.org/r/69122/diff/1/
> 
> 
> Testing
> ---
> 
> add new test case to check authorization based on user name case
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69122: SENTRY-2432: The case of a username is ignored when determining object ownership

[Na Li via Review Board]

> On Oct. 23, 2018, 3:42 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 925 (original), 928 (patched)
> > 
> >
> > Here?

what do you mean "Here?"?


- Na


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


On Oct. 22, 2018, 10:04 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69122/
> ---
> 
> (Updated Oct. 22, 2018, 10:04 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2432
> https://issues.apache.org/jira/browse/sentry-2432
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Keep user name case when saving into sentry DB and fetching privileges for 
> that user.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  f29c455 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  b387a22 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  4a9afe3 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
>  d3294f4 
> 
> 
> Diff: https://reviews.apache.org/r/69122/diff/1/
> 
> 
> Testing
> ---
> 
> add new test case to check authorization based on user name case
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69122: SENTRY-2432: The case of a username is ignored when determining object ownership

[Na Li via Review Board]

> On Oct. 23, 2018, 3:42 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 925 (original), 928 (patched)
> > 
> >
> > Here?
> 
> Na Li wrote:
> what do you mean "Here?"?
> 
> Arjun Mishra wrote:
> I mean in the method "alterSentryUserGrantPrivileges" we don't trim the 
> userName before doing any grant

userName is trimmed in getMSentryUserByName


> On Oct. 23, 2018, 3:42 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 984 (original), 990 (patched)
> > 
> >
> > Here?
> 
> Na Li wrote:
> what do you mean?

userName is trimmed in getMSentryUserByName


- Na


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


On Oct. 22, 2018, 10:04 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69122/
> ---
> 
> (Updated Oct. 22, 2018, 10:04 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2432
> https://issues.apache.org/jira/browse/sentry-2432
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Keep user name case when saving into sentry DB and fetching privileges for 
> that user.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  f29c455 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  b387a22 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  4a9afe3 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
>  d3294f4 
> 
> 
> Diff: https://reviews.apache.org/r/69122/diff/1/
> 
> 
> Testing
> ---
> 
> add new test case to check authorization based on user name case
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69122: SENTRY-2432: The case of a username is ignored when determining object ownership

[Na Li via Review Board]

> On Oct. 23, 2018, 3:44 p.m., Arjun Mishra wrote:
> > Let me know what you think about this. I think we could minimize the change 
> > set by keeping changes to the core methods? alterSentryRevokePrivilegeCore 
> > and alterSentryGrantPrivilegeCore? 
> > For example there are methods like dropOrRenamePrivilegeForAllEntities, and 
> > importRolePrivilegeMapping that don't have a trim and lower change to it

agree. It is cleaner and can make sure we process input name correctly. I will 
make change accordingly


- Na


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


On Oct. 22, 2018, 10:04 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69122/
> ---
> 
> (Updated Oct. 22, 2018, 10:04 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2432
> https://issues.apache.org/jira/browse/sentry-2432
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Keep user name case when saving into sentry DB and fetching privileges for 
> that user.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  f29c455 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  b387a22 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  4a9afe3 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
>  d3294f4 
> 
> 
> Diff: https://reviews.apache.org/r/69122/diff/1/
> 
> 
> Testing
> ---
> 
> add new test case to check authorization based on user name case
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69122: SENTRY-2432: The case of a username is ignored when determining object ownership

[Na Li via Review Board]

> On Oct. 23, 2018, 3:42 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 2837 (original), 2847 (patched)
> > 
> >
> > What about this method? trim and lower?

good catch! I will add trim and lower in alterSentryGrantPrivilegeCore


- Na


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


On Oct. 22, 2018, 10:04 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69122/
> ---
> 
> (Updated Oct. 22, 2018, 10:04 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2432
> https://issues.apache.org/jira/browse/sentry-2432
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Keep user name case when saving into sentry DB and fetching privileges for 
> that user.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  f29c455 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  b387a22 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  4a9afe3 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
>  d3294f4 
> 
> 
> Diff: https://reviews.apache.org/r/69122/diff/1/
> 
> 
> Testing
> ---
> 
> add new test case to check authorization based on user name case
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69204: SENTRY-2437: When granting privileges a single transaction per grant causes long delays

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Oct. 29, 2018, 9:35 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69204/
> ---
> 
> (Updated Oct. 29, 2018, 9:35 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2437
> https://issues.apache.org/jira/browse/SENTRY-2437
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently sentry creates a transaction for each TSentryPrivilege object it 
> needs to grant. If the list of privileges is very large creating a single 
> transaction for each significantly affects performance. This is particularly 
> impactful for tables with large columns and if a user grants privileges to 
> many of those columns
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  01b363479 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  97407fff5 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
>  1d87b0b66 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  f2f38a38f 
> 
> 
> Diff: https://reviews.apache.org/r/69204/diff/2/
> 
> 
> Testing
> ---
> 
> $ mvn -f sentry-service/sentry-service-server/pom.xml test 
> -Dtest=TestSentryStore
> $ mvn -f sentry-service/sentry-service-server/pom.xml test 
> -Dtest=TestHMSFollowerSentryStoreIntegration
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69087: SENTRY-2249: Enable batch insert of HMS paths in Full Snapshot.

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
Lines 303 (patched)


why don't you replace MPath with MPathToPersist? 

In your current approach, two classes mapped into the same DB table and 
both use datastore to generate PATH_ID. Is it possible to have duplicate 
PATH_ID and cause persiting entry in AUTHZ_PATH to fail?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3316 (patched)


can you use nextObjectId = getNextAuthzObjectID() and change line 3324 with 
"nextObjectId ++" to be consistent with line 3447?


- Na Li


On Oct. 31, 2018, 12:58 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69087/
> ---
> 
> (Updated Oct. 31, 2018, 12:58 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2249
> https://issues.apache.org/jira/browse/SENTRY-2249
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently each entry in full snapshot of HMS is persisted one entry at a 
> time. Instead it could be optimized by persisting the path entries in 
> batches. DB operations are expensive, reducing the number of database 
> operations and around trip time will help. This would decrease the time to 
> persist the snapshot in to database significantly.
> 
> Size of the batch could be configurable.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  c51f25a0393b482814afcd3b7a19e547b689ac6e 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPathToPersist.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  20ec0deab6b97065cfe99beea3d14a6c7268aac3 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  33c40613a05f7c7fde314af6aba6b269bf6ffaae 
> 
> 
> Diff: https://reviews.apache.org/r/69087/diff/2/
> 
> 
> Testing
> ---
> 
> Made sure all the unit tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Review Request 69175: SENTRY-2433: Dropping object privileges does not include update of dropping user privileges

[Na Li via Review Board]

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

Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
Pena.


Bugs: sentry-2433
https://issues.apache.org/jira/browse/sentry-2433


Repository: sentry


Description
---

1) use PermissionsUpdate.ALL_PRIVS in "1.1) NotificationProcessor.java static 
Update getPermUpdatableOnDrop(TSentryAuthorizable authorizable)" and 
"SentryPlugin.java public Update onDropSentryPrivilege(TDropPrivilegesRequest 
request)" instead of "PermissionsUpdate.ALL_ROLES"
2) check PermissionsUpdate.ALL_PRIVS instead of PermissionsUpdate.ALL_ROLES in 
UpdateableAuthzPermissions.applyPrivilegeUpdates() in 
"pUpdate.getDelPrivileges()" processing.


Diffs
-

  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
 c87d205 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
 b8f5ce7 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
 7b7d0e1 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
 29d2256 


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


Testing
---

owner privilege tests pass and add new test for external table


Thanks,

Na Li

Re: Review Request 69212: SENTRY-2329: Integrate sentry with Hadoop 3.1.1

[Na Li via Review Board]

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




sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
Line 143 (original)


why do you remove it? Is it impossible to throw this exception?


- Na Li


On Oct. 30, 2018, 2:16 a.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69212/
> ---
> 
> (Updated Oct. 30, 2018, 2:16 a.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2329
> https://issues.apache.org/jira/browse/sentry-2329
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Bump hadoop.version to 3.1.1. The changes on Sentry are compatible with 
> Hadoop 2.7.
> 
> 
> Diffs
> -
> 
>   pom.xml acbdcc2722bf189811cb528ac1b2d07983a571c2 
>   sentry-binding/sentry-binding-solr/pom.xml 
> f2a5fca76d3d220fcf2b72a3179ff5218fc6577c 
>   sentry-hdfs/sentry-hdfs-common/pom.xml 
> df6f04c048b502ff5f8e8ec397d75166faba8c3c 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  c9ecc4021b167d98c7dade409c97ae7d26e967ea 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryINodeAttributesProvider.java
>  18b62652a11dfee6683cb8f24944ccd3d344dc9f 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryWebServerWithKerberos.java
>  5d94d4bc6a2a47189e69556a5e4d9bdee05952a7 
> 
> 
> Diff: https://reviews.apache.org/r/69212/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 69175: SENTRY-2433: Dropping object privileges does not include update of dropping user privileges

[Na Li via Review Board]

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

(Updated Oct. 29, 2018, 5:07 p.m.)


Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
Pena.


Bugs: sentry-2433
https://issues.apache.org/jira/browse/sentry-2433


Repository: sentry


Description
---

1) use PermissionsUpdate.ALL_PRIVS in "1.1) NotificationProcessor.java static 
Update getPermUpdatableOnDrop(TSentryAuthorizable authorizable)" and 
"SentryPlugin.java public Update onDropSentryPrivilege(TDropPrivilegesRequest 
request)" instead of "PermissionsUpdate.ALL_ROLES"
2) check PermissionsUpdate.ALL_PRIVS instead of PermissionsUpdate.ALL_ROLES in 
UpdateableAuthzPermissions.applyPrivilegeUpdates() in 
"pUpdate.getDelPrivileges()" processing.


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
 c87d205 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
 0f3c162 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
 7b7d0e1 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
 29d2256 


Diff: https://reviews.apache.org/r/69175/diff/2/

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


Testing
---

owner privilege tests pass and add new test for external table


Thanks,

Na Li

Re: Review Request 68890: SENTRY-2300: Move Permission Update due to DDL to HMS Post Event Listener

[Na Li via Review Board]

> On Oct. 9, 2018, 2:13 p.m., kalyan kumar kalvagadda wrote:
> > Lina,
> > 
> > I see advantages of this approach but i also see some dis-advantages which 
> > are serious.
> > 
> > 1. If there is a failure while handling the notification for any reason 
> > there is no way to retry. This is not the case if we process the event from 
> > notification log as HMSfollower would not update the notification ID in the 
> > database unless it is process. This will force HMSFollower to fetch it 
> > again and process it.
> > 2. We know that customers are observing issues with sync listener and are 
> > used to disable the sync listerner when timeut exceptions are observed. We 
> > need to unerstand one thing. Root cause for this delay in processing the 
> > notifications. This delay could also be because of delay in updating the 
> > permissions as well. Unless we know why are the notifications processed 
> > slowly, moving the logic of updating the sentry permissions synconously 
> > would effect a lot of customer as their jobs gets slowdown. That was the 
> > case before.

Kalyan,

for your concerns 1) "If there is a failure while handling the notification for 
any reason there is no way to retry. This is not the case if we process the 
event from notification log as HMSfollower would not update the notification ID 
in the database unless it is process. This will force HMSFollower to fetch it 
again and process it.", I copy my response in Jira sentry-2300 here 

[Lina] This is not true. With current approach, if processing the notification 
event fails, the event ID is saved and no longer processed again and you made 
that code change avoid getting stuck at illegal event (what you stated above is 
that the event will be refectched and processed again), same as the new 
approach I am taking. 
see 
https://github.com/apache/sentry/blob/master/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java#L493

  public void processNotifications(Collection events) throws 
Exception {
boolean isNotificationProcessed;
if (events.isEmpty()) {
  return;
}

for (NotificationEvent event : events) {
  isNotificationProcessed = false;
  try {
// Only the leader should process the notifications
if (!isLeader()) {
  LOGGER.debug("Not processing notifications since not a leader");
  return;
}
isNotificationProcessed = 
notificationProcessor.processNotificationEvent(event);
  } catch (Exception e) {
if (e.getCause() instanceof JDODataStoreException) {
  LOGGER.info("Received JDO Storage Exception, Could be because of 
processing "
  + "duplicate notification");
  if (event.getEventId() <= 
sentryStore.getLastProcessedNotificationID()) {
// Rest of the notifications need not be processed.
LOGGER.error("Received event with Id: {} which is smaller then the 
ID "
+ "persisted in store", event.getEventId());
break;
  }
} else {
  LOGGER.error("Processing the notification with ID:{} failed with 
exception {}",
  event.getEventId(), e);
}
  }
  if (!isNotificationProcessed) {
try {
  // Update the notification ID in the persistent store even when the 
notification is<-- if the processing of 
event fails, the notification ID is still saved and not fetched again
  // not processed as the content in in the notification is not valid.
  // Continue processing the next notification.
  LOGGER.debug("Explicitly Persisting Notification ID = {} ", 
event.getEventId());
  sentryStore.persistLastProcessedNotificationID(event.getEventId());
} catch (Exception failure) {
  LOGGER.error("Received exception while persisting the notification ID 
= {}", event.getEventId());
  throw failure;
}
  }
  // Wake up any HMS waiters that are waiting for this ID.
  wakeUpWaitingClientsForSync(event.getEventId());
}
}


- Na


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


On Oct. 2, 2018, 9:26 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68890/
> ---
> 
> (Updated Oct. 2, 2018, 9:26 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2300
> https://issues.apache.org/jira/browse/sentry-2300
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Move the privilege updates from Notification event processing at 
> 

Re: Review Request 68890: SENTRY-2300: Move Permission Update due to DDL to HMS Post Event Listener

[Na Li via Review Board]

> On Oct. 4, 2018, 6:26 p.m., Arjun Mishra wrote:
> > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegePrincipalType.java
> > Line 36 (original), 36 (patched)
> > 
> >
> > Can you revert changes to this file? Nothing has changed here?

It is auto-generated by thrift.


- Na


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


On Oct. 2, 2018, 9:26 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68890/
> ---
> 
> (Updated Oct. 2, 2018, 9:26 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2300
> https://issues.apache.org/jira/browse/sentry-2300
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Move the privilege updates from Notification event processing at 
> NotificationProcessor to HMS post event processing at 
> SentryPolicyStoreProcessor
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
>  e0f35e8 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
>  c37f370 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java
>  98de8e3 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegePrincipalType.java
>  6eb8521 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java
>  7cdf148 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java
>  5fc299b 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  68d864c 
>   
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
>  3364648 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  709434c 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
>  7b7d0e1 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  059621a 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  0d62941 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
>  a886836 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestNotificationProcessor.java
>  f227bb4 
> 
> 
> Diff: https://reviews.apache.org/r/68890/diff/2/
> 
> 
> Testing
> ---
> 
> Add new test cases in TestSentryPolicyStoreProcessor, and update tests in 
> TestHMSFollower and TestNotificationProcessor. Tests in 
> TestSentryPolicyStoreProcessor, TestHMSFollower and TestNotificationProcessor 
> passed.
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 68918: SENTRY-2423: Increase the allocation size for auto-increment of id's for Snapshot tables.

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Oct. 3, 2018, 8:24 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68918/
> ---
> 
> (Updated Oct. 3, 2018, 8:24 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2423
> https://issues.apache.org/jira/browse/SENTRY-2423
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently sentry uses native strategy for auto generation of ID columns for 
> which the allocation can not be increased.
> This should be change to "increment" strategy and which lets to configure the 
> allocation size. This reduces the delay caused for checking the 
> SEQUENCE_TABLE.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  e90fe2df46266d72f1eb3706b948fb163a44c4a1 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  6539e33e021e0f5acaa7827356a8e9b3e4286b18 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1722109e381bc7be81a4673d12c1ac9f81195c47 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  a299e008cc0df92f3d692024c7794aa494b64d2d 
> 
> 
> Diff: https://reviews.apache.org/r/68918/diff/1/
> 
> 
> Testing
> ---
> 
> Made sure all the existing tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68890: SENTRY-2300: Move Permission Update due to DDL to HMS Post Event Listener

[Na Li via Review Board]

> On Oct. 9, 2018, 2:13 p.m., kalyan kumar kalvagadda wrote:
> > Lina,
> > 
> > I see advantages of this approach but i also see some dis-advantages which 
> > are serious.
> > 
> > 1. If there is a failure while handling the notification for any reason 
> > there is no way to retry. This is not the case if we process the event from 
> > notification log as HMSfollower would not update the notification ID in the 
> > database unless it is process. This will force HMSFollower to fetch it 
> > again and process it.
> > 2. We know that customers are observing issues with sync listener and are 
> > used to disable the sync listerner when timeut exceptions are observed. We 
> > need to unerstand one thing. Root cause for this delay in processing the 
> > notifications. This delay could also be because of delay in updating the 
> > permissions as well. Unless we know why are the notifications processed 
> > slowly, moving the logic of updating the sentry permissions synconously 
> > would effect a lot of customer as their jobs gets slowdown. That was the 
> > case before.
> 
> Na Li wrote:
> Kalyan,
> 
> for your concerns 1) "If there is a failure while handling the 
> notification for any reason there is no way to retry. This is not the case if 
> we process the event from notification log as HMSfollower would not update 
> the notification ID in the database unless it is process. This will force 
> HMSFollower to fetch it again and process it.", I copy my response in Jira 
> sentry-2300 here 
> 
> [Lina] This is not true. With current approach, if processing the 
> notification event fails, the event ID is saved and no longer processed again 
> and you made that code change avoid getting stuck at illegal event (what you 
> stated above is that the event will be refectched and processed again), same 
> as the new approach I am taking. 
> see 
> https://github.com/apache/sentry/blob/master/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java#L493
> 
>   public void processNotifications(Collection events) 
> throws Exception {
> boolean isNotificationProcessed;
> if (events.isEmpty()) {
>   return;
> }
> 
> for (NotificationEvent event : events) {
>   isNotificationProcessed = false;
>   try {
> // Only the leader should process the notifications
> if (!isLeader()) {
>   LOGGER.debug("Not processing notifications since not a leader");
>   return;
> }
> isNotificationProcessed = 
> notificationProcessor.processNotificationEvent(event);
>   } catch (Exception e) {
> if (e.getCause() instanceof JDODataStoreException) {
>   LOGGER.info("Received JDO Storage Exception, Could be because 
> of processing "
>   + "duplicate notification");
>   if (event.getEventId() <= 
> sentryStore.getLastProcessedNotificationID()) {
> // Rest of the notifications need not be processed.
> LOGGER.error("Received event with Id: {} which is smaller 
> then the ID "
> + "persisted in store", event.getEventId());
> break;
>   }
> } else {
>   LOGGER.error("Processing the notification with ID:{} failed 
> with exception {}",
>   event.getEventId(), e);
> }
>   }
>   if (!isNotificationProcessed) {
> try {
>   // Update the notification ID in the persistent store even when 
> the notification is<-- if the processing 
> of event fails, the notification ID is still saved and not fetched again
>   // not processed as the content in in the notification is not 
> valid.
>   // Continue processing the next notification.
>   LOGGER.debug("Explicitly Persisting Notification ID = {} ", 
> event.getEventId());
>   
> sentryStore.persistLastProcessedNotificationID(event.getEventId());
> } catch (Exception failure) {
>   LOGGER.error("Received exception while persisting the 
> notification ID = {}", event.getEventId());
>   throw failure;
> }
>   }
>   // Wake up any HMS waiters that are waiting for this ID.
>   wakeUpWaitingClientsForSync(event.getEventId());
> }
> }

Kalyan,

for your concern 2) "We need to unerstand one thing. Root cause for this delay 
in processing the notifications."
One major reason is getting full snapshot or notification event takes too long. 
HMS has to wait for sentry to get notification events from HMS. It causes nesty 
dependency circle. It takes at least 0-500 ms delay for HMS to get back from 
waiting for sentry getting notification asynchronously. You can ask Arjun to 
confirm that.


- Na

Re: Review Request 68890: SENTRY-2300: Move Permission Update due to DDL to HMS Post Event Listener

[Na Li via Review Board]

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

(Updated Oct. 2, 2018, 9:26 p.m.)


Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
Pena.


Bugs: sentry-2300
https://issues.apache.org/jira/browse/sentry-2300


Repository: sentry


Description
---

Move the privilege updates from Notification event processing at 
NotificationProcessor to HMS post event processing at SentryPolicyStoreProcessor


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
 e0f35e8 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
 c37f370 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java
 98de8e3 
  
sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegePrincipalType.java
 6eb8521 
  
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java
 7cdf148 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java
 5fc299b 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 68d864c 
  
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
 3364648 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 709434c 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
 7b7d0e1 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
 059621a 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
 0d62941 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
 a886836 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestNotificationProcessor.java
 f227bb4 


Diff: https://reviews.apache.org/r/68890/diff/2/

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


Testing
---

Add new test cases in TestSentryPolicyStoreProcessor, and update tests in 
TestHMSFollower and TestNotificationProcessor. Tests in 
TestSentryPolicyStoreProcessor, TestHMSFollower and TestNotificationProcessor 
passed.


Thanks,

Na Li

Review Request 68982: SENTRY-2426: Update the version in pom file at 2.1 branch

[Na Li via Review Board]

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

Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
Pena.


Bugs: sentry-2426
https://issues.apache.org/jira/browse/sentry-2426


Repository: sentry


Description
---

update the version from 2.1.0 to 2.1.1-SNAPSHOT in the pom files of branch-2.1 
for maintenance release


Diffs
-

  pom.xml 73112e8 
  sentry-binding/pom.xml 8784794 
  sentry-binding/sentry-binding-hbase-indexer/pom.xml e0cb32c 
  sentry-binding/sentry-binding-hive-common/pom.xml cfa5081 
  sentry-binding/sentry-binding-hive-conf/pom.xml c9f6fcc 
  sentry-binding/sentry-binding-hive-follower/pom.xml 5e51cc2 
  sentry-binding/sentry-binding-hive/pom.xml 6f4d69e 
  sentry-binding/sentry-binding-kafka/pom.xml 27b0ad9 
  sentry-binding/sentry-binding-solr/pom.xml cba7d59 
  sentry-binding/sentry-binding-sqoop/pom.xml 37c02f9 
  sentry-core/pom.xml e536805 
  sentry-core/sentry-core-common/pom.xml 07b6bcb 
  sentry-core/sentry-core-model-db/pom.xml 1e0b02d 
  sentry-core/sentry-core-model-indexer/pom.xml 1ccc8e8 
  sentry-core/sentry-core-model-kafka/pom.xml 6a7a989 
  sentry-core/sentry-core-model-solr/pom.xml defeaba 
  sentry-core/sentry-core-model-sqoop/pom.xml 9ddfd1b 
  sentry-dist/pom.xml c795809 
  sentry-hdfs/pom.xml 358350e 
  sentry-hdfs/sentry-hdfs-common/pom.xml f8fa887 
  sentry-hdfs/sentry-hdfs-dist/pom.xml 123f631 
  sentry-hdfs/sentry-hdfs-namenode-plugin/pom.xml 716789b 
  sentry-hdfs/sentry-hdfs-service/pom.xml bcdf8c3 
  sentry-policy/pom.xml 89cf602 
  sentry-policy/sentry-policy-common/pom.xml c2f0075 
  sentry-policy/sentry-policy-engine/pom.xml 6c1576a 
  sentry-provider/pom.xml 814fe45 
  sentry-provider/sentry-provider-cache/pom.xml ec43c05 
  sentry-provider/sentry-provider-common/pom.xml a733eaf 
  sentry-provider/sentry-provider-db/pom.xml 6d2590e 
  sentry-provider/sentry-provider-file/pom.xml 78e603d 
  sentry-service/pom.xml 565e86b 
  sentry-service/sentry-service-api/pom.xml 465ccf2 
  sentry-service/sentry-service-client/pom.xml 8249b20 
  sentry-service/sentry-service-server/pom.xml 7729656 
  sentry-solr/pom.xml 218c73e 
  sentry-solr/solr-sentry-handlers/pom.xml 4f9e4ba 
  sentry-spi/pom.xml eb9e668 
  sentry-tests/pom.xml 1b6bd0d 
  sentry-tests/sentry-tests-hive/pom.xml d9e535d 
  sentry-tests/sentry-tests-kafka/pom.xml c6c0565 
  sentry-tests/sentry-tests-solr/pom.xml cdf2924 
  sentry-tests/sentry-tests-sqoop/pom.xml 2a999f5 
  sentry-thirdparty/pom.xml 7c0fc9d 
  sentry-thirdparty/sentry-shaded/pom.xml 99a9217 
  sentry-tools/pom.xml 4bf374c 


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


Testing
---


Thanks,

Na Li

Re: Review Request 68973: SENTRY-2305: Optimize time taken for persistence HMS snapshot by persisting in parallel

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3421 (patched)


can we wrap the whole processing in try() and wait for threadpool to 
shutdown in finally?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3430 (patched)


to reduce overhead of reading when persisting snapshot fails, can we just 
delete all path mapping with nextSnapshotID?


- Na Li


On Oct. 10, 2018, 7:05 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68973/
> ---
> 
> (Updated Oct. 10, 2018, 7:05 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2305
> https://issues.apache.org/jira/browse/SENTRY-2305
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> I have considered multiple options. Persisting in batches is not an option 
> with out changing the schema as the data nucleus does not persist row in 
> batches for tables which have foreign key on other tables.
> 
> I see that best option is to persist the paths in parallel. It gave good 
> results.
> 
> Solution Approach:
> 
> I have used a thread pool to persist the snapshot. Size of this thread pool 
> is configurable. Paths for each object database/table are submitted to this 
> thread pool. If for reason some of the paths are not pesisted, snapshot is 
> removed and exception is throw back.
> 
>   This patch along with SENTRY-2423 was 5 times faster when tested with below.
> 
>  
> 
> Object Type   Count
> Databases 209
> Tables2100
> Partitions24
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a736ca9604eb0bb182a159b5a2aed274275c16e 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  0d62941a7bd45e0d8a67b5d95fdb39a8801f5a26 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  4a9afe303672baff39be01d4f190034b2bfb75fe 
> 
> 
> Diff: https://reviews.apache.org/r/68973/diff/2/
> 
> 
> Testing
> ---
> 
> Added new test and also made sure that existing tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68968: SENTRY-2425: Add metric to track the time taken to update the owner privilege

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
Lines 124 (patched)


We should use different metric for "granting" vs "update" since update is 
much more complicated and would take more time. Once we support "revoke" owner 
privilege, we can add a metric for that.


- Na Li


On Oct. 9, 2018, 6:01 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68968/
> ---
> 
> (Updated Oct. 9, 2018, 6:01 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2425
> https://issues.apache.org/jira/browse/SENTRY-2425
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> New metric should be added to track the time taken by sentry server in 
> granting/updating owner privileges.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  d34340cd6a41ba01943bd3b2b42c9bddc9fa722f 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  709434c388689b63d5db7d71cd6cc47952d647bf 
> 
> 
> Diff: https://reviews.apache.org/r/68968/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68958: SENTRY-2419: Log where sentry stands in the process of persisting the snpashot

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 487 (patched)


log message is not right. Fix it.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 501 (patched)


log message is not right. Fix it.


- Na Li


On Oct. 8, 2018, 10:16 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68958/
> ---
> 
> (Updated Oct. 8, 2018, 10:16 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2419
> https://issues.apache.org/jira/browse/SENTRY-2419
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Process of persisting the snapshot might take longer based on the snapshot 
> size. It would be helpfull to to be able to understand where the sentry 
> sentry stands in the process of persisting.
> 
>  
> 
> Adding a Metric for this will be helpfull in debugging.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  e90fe2df4 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  d34340cd6 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  b6dca7aa8 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1722109e3 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  3a68eb6bf 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  059621a68 
> 
> 
> Diff: https://reviews.apache.org/r/68958/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68982: SENTRY-2426: Update the version in pom file at 2.1 branch

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Oct. 10, 2018, 9:42 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68982/
> ---
> 
> (Updated Oct. 10, 2018, 9:42 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2426
> https://issues.apache.org/jira/browse/sentry-2426
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> update the version from 2.1.0 to 2.1.1-SNAPSHOT in the pom files of 
> branch-2.1 for maintenance release
> 
> 
> Diffs
> -
> 
>   pom.xml 73112e8 
>   sentry-binding/pom.xml 8784794 
>   sentry-binding/sentry-binding-hbase-indexer/pom.xml e0cb32c 
>   sentry-binding/sentry-binding-hive-common/pom.xml cfa5081 
>   sentry-binding/sentry-binding-hive-conf/pom.xml c9f6fcc 
>   sentry-binding/sentry-binding-hive-follower/pom.xml 5e51cc2 
>   sentry-binding/sentry-binding-hive/pom.xml 6f4d69e 
>   sentry-binding/sentry-binding-kafka/pom.xml 27b0ad9 
>   sentry-binding/sentry-binding-solr/pom.xml cba7d59 
>   sentry-binding/sentry-binding-sqoop/pom.xml 37c02f9 
>   sentry-core/pom.xml e536805 
>   sentry-core/sentry-core-common/pom.xml 07b6bcb 
>   sentry-core/sentry-core-model-db/pom.xml 1e0b02d 
>   sentry-core/sentry-core-model-indexer/pom.xml 1ccc8e8 
>   sentry-core/sentry-core-model-kafka/pom.xml 6a7a989 
>   sentry-core/sentry-core-model-solr/pom.xml defeaba 
>   sentry-core/sentry-core-model-sqoop/pom.xml 9ddfd1b 
>   sentry-dist/pom.xml c795809 
>   sentry-hdfs/pom.xml 358350e 
>   sentry-hdfs/sentry-hdfs-common/pom.xml f8fa887 
>   sentry-hdfs/sentry-hdfs-dist/pom.xml 123f631 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/pom.xml 716789b 
>   sentry-hdfs/sentry-hdfs-service/pom.xml bcdf8c3 
>   sentry-policy/pom.xml 89cf602 
>   sentry-policy/sentry-policy-common/pom.xml c2f0075 
>   sentry-policy/sentry-policy-engine/pom.xml 6c1576a 
>   sentry-provider/pom.xml 814fe45 
>   sentry-provider/sentry-provider-cache/pom.xml ec43c05 
>   sentry-provider/sentry-provider-common/pom.xml a733eaf 
>   sentry-provider/sentry-provider-db/pom.xml 6d2590e 
>   sentry-provider/sentry-provider-file/pom.xml 78e603d 
>   sentry-service/pom.xml 565e86b 
>   sentry-service/sentry-service-api/pom.xml 465ccf2 
>   sentry-service/sentry-service-client/pom.xml 8249b20 
>   sentry-service/sentry-service-server/pom.xml 7729656 
>   sentry-solr/pom.xml 218c73e 
>   sentry-solr/solr-sentry-handlers/pom.xml 4f9e4ba 
>   sentry-spi/pom.xml eb9e668 
>   sentry-tests/pom.xml 1b6bd0d 
>   sentry-tests/sentry-tests-hive/pom.xml d9e535d 
>   sentry-tests/sentry-tests-kafka/pom.xml c6c0565 
>   sentry-tests/sentry-tests-solr/pom.xml cdf2924 
>   sentry-tests/sentry-tests-sqoop/pom.xml 2a999f5 
>   sentry-thirdparty/pom.xml 7c0fc9d 
>   sentry-thirdparty/sentry-shaded/pom.xml 99a9217 
>   sentry-tools/pom.xml 4bf374c 
> 
> 
> Diff: https://reviews.apache.org/r/68982/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 68973: SENTRY-2305: Optimize time taken for persistence HMS snapshot by persisting in parallel

[Na Li via Review Board]

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



do you have measurement on how much time saved on saving big snapshot in 
parallel vs the number of threads? When multiple threads accessing the same 
table, the improvement may not be linear.

- Na Li


On Oct. 10, 2018, 11:04 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68973/
> ---
> 
> (Updated Oct. 10, 2018, 11:04 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2305
> https://issues.apache.org/jira/browse/SENTRY-2305
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> I have considered multiple options. Persisting in batches is not an option 
> with out changing the schema as the data nucleus does not persist row in 
> batches for tables which have foreign key on other tables.
> 
> I see that best option is to persist the paths in parallel. It gave good 
> results.
> 
> Solution Approach:
> 
> I have used a thread pool to persist the snapshot. Size of this thread pool 
> is configurable. Paths for each object database/table are submitted to this 
> thread pool. If for reason some of the paths are not pesisted, snapshot is 
> removed and exception is throw back.
> 
>   This patch along with SENTRY-2423 was 5 times faster when tested with below.
> 
>  
> 
> Object Type   Count
> Databases 209
> Tables2100
> Partitions24
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a736ca9604eb0bb182a159b5a2aed274275c16e 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  0d62941a7bd45e0d8a67b5d95fdb39a8801f5a26 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  4a9afe303672baff39be01d4f190034b2bfb75fe 
> 
> 
> Diff: https://reviews.apache.org/r/68973/diff/3/
> 
> 
> Testing
> ---
> 
> Added new test and also made sure that existing tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68918: SENTRY-2423: Increase the allocation size for auto-increment of id's for Snapshot tables.

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 4575 (patched)


do you have measurement on how long it takes with/without this fix?


- Na Li


On Oct. 3, 2018, 8:24 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68918/
> ---
> 
> (Updated Oct. 3, 2018, 8:24 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2423
> https://issues.apache.org/jira/browse/SENTRY-2423
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently sentry uses native strategy for auto generation of ID columns for 
> which the allocation can not be increased.
> This should be change to "increment" strategy and which lets to configure the 
> allocation size. This reduces the delay caused for checking the 
> SEQUENCE_TABLE.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  e90fe2df46266d72f1eb3706b948fb163a44c4a1 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  6539e33e021e0f5acaa7827356a8e9b3e4286b18 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1722109e381bc7be81a4673d12c1ac9f81195c47 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  a299e008cc0df92f3d692024c7794aa494b64d2d 
> 
> 
> Diff: https://reviews.apache.org/r/68918/diff/1/
> 
> 
> Testing
> ---
> 
> Made sure all the existing tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Review Request 68890: SENTRY-2300: Move Permission Update due to DDL to HMS Post Event Listener

[Na Li via Review Board]

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

Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
Pena.


Bugs: sentry-2300
https://issues.apache.org/jira/browse/sentry-2300


Repository: sentry


Description
---

Move the privilege updates from Notification event processing at 
NotificationProcessor to HMS post event processing at SentryPolicyStoreProcessor


Diffs
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
 e0f35e8 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
 c37f370 
  
sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegePrincipalType.java
 6eb8521 
  
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java
 7cdf148 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java
 5fc299b 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 68d864c 
  
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
 3364648 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 709434c 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
 7b7d0e1 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
 059621a 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
 0d62941 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestNotificationProcessor.java
 f227bb4 


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


Testing
---

Add new test cases in TestSentryPolicyStoreProcessor, and update tests in 
TestHMSFollower and TestNotificationProcessor. Tests in 
TestSentryPolicyStoreProcessor, TestHMSFollower and TestNotificationProcessor 
passed.


Thanks,

Na Li

Re: Review Request 68779: SENTRY-2409: ALTER TABLE SET OWNER does not allow to change the table if using only the table name

[Na Li via Review Board]

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

(Updated Oct. 1, 2018, 9:23 p.m.)


Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
Pena.


Bugs: sentry-2409
https://issues.apache.org/jira/browse/sentry-2409


Repository: sentry


Description
---

add code to get table name and DB name based on token children size


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
 6731d1a 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
 385ca98 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
 55a79ee 


Diff: https://reviews.apache.org/r/68779/diff/2/

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


Testing
---

unit tests in TestOwnerPrivileges passed


Thanks,

Na Li

Re: Review Request 68779: SENTRY-2409: ALTER TABLE SET OWNER does not allow to change the table if using only the table name

[Na Li via Review Board]

> On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
> > Lines 680 (patched)
> > 
> >
> > Do you need to insert data for this test case? I don't think is 
> > necessary. Inserts are expensive in Hive, so removing it will save time on 
> > the test execution.

I will remove it


> On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
> > Lines 682 (patched)
> > 
> >
> > Code convention: separate if and ( by a space.
> > 
> > Btw, I noticed that this test case has two different test paths 
> > depending on the value of this flag. I don't see this a good practice when 
> > writing test cases because if you change the flag for an unknown reason, 
> > then you will never execute the other test case and you won't know if it 
> > fails.
> > 
> > I'd rather write another test case for the grant option enabled instead 
> > of putting logic conditions in test cases.

I will split the test into two test cases: one for positive and one for negative


> On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
> > Lines 710-717 (patched)
> > 
> >
> > This test case is a negative test case and it can be put in different 
> > method.

will split it out


> On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
> > Lines 716-717 (patched)
> > 
> >
> > How do we validate that the error was not authorized and it was not a 
> > syntax error?

will use HiveSqlException


> On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
> > Lines 725 (patched)
> > 
> >
> > This line fails immediatly, but instead of throwing a test case error, 
> > it goes to the finally() clause and finish with the test successfully.

why do you think this fails? It should work.


> On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
> > Lines 733 (patched)
> > 
> >
> > Aren't we running this test case in other parts of this class? Seems 
> > redundant to test that the ALTER TABLE fails with a table that does not 
> > exist. 
> > 
> > I prefer to see different methods for each test case you're trying to 
> > validate instead of putting several into one method.

I will remove it


- Na


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


On Sept. 20, 2018, 4:24 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68779/
> ---
> 
> (Updated Sept. 20, 2018, 4:24 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2409
> https://issues.apache.org/jira/browse/sentry-2409
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> add code to get table name and DB name based on token children size
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  6731d1a 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
>  55a79ee 
> 
> 
> Diff: https://reviews.apache.org/r/68779/diff/1/
> 
> 
> Testing
> ---
> 
> unit tests in TestOwnerPrivileges passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 68788: SENTRY-2413: Provide a configuration option to permit specific DB privileges to be granted explicitly

[Na Li via Review Board]

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




sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
Lines 53 (patched)


You can use

Assert.fail("ALL, SELECT and INSERT privileges should be permitted");

It is more intuitive.



sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
Lines 60 (patched)


same thing Assert.fail()



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 264 (patched)


Can you add e2e test case for this? To make sure this is called when client 
requests some privileges not supported?


- Na Li


On Sept. 20, 2018, 8:28 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68788/
> ---
> 
> (Updated Sept. 20, 2018, 8:28 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: sentry-2413
> https://issues.apache.org/jira/browse/sentry-2413
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add a new configuration 'sentry.db.explicit.grants.permitted' that accepts a 
> comma-separated list of privileges that can be granted by Sentry DB clients. 
> If the value is empty, then any privilege can be granted as it works normally 
> (this is the default behavior).
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  adc1947a1 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  8cdbde4f9 
>   
> sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  3a9623b46 
> 
> 
> Diff: https://reviews.apache.org/r/68788/diff/1/
> 
> 
> Testing
> ---
> 
> Unit tests added.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68787: SENTRY-2406: Make sure inputHierarchy and outputHierarchy have unique values

[Na Li via Review Board]

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



can you add unit test?

- Na Li


On Sept. 20, 2018, 7:32 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68787/
> ---
> 
> (Updated Sept. 20, 2018, 7:32 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2406
> https://issues.apache.org/jira/browse/SENTRY-2406
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When building input or output hierarchy list, we iterate over all ReadEntity 
> inputs received from Hive. The inputs particularly have accessed columns that 
> can repeat for other ReadEntity objects. This happens definitively when a 
> table has partitions. We should in general protect Sentry from not having to 
> authorize over a list of DBModelAuthorizable objects when it has already been 
> done.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  da1956b8e 
> 
> 
> Diff: https://reviews.apache.org/r/68787/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Review Request 68779: SENTRY-2409: ALTER TABLE SET OWNER does not allow to change the table if using only the table name

[Na Li via Review Board]

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

Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
Pena.


Bugs: sentry-2409
https://issues.apache.org/jira/browse/sentry-2409


Repository: sentry


Description
---

add code to get table name and DB name based on token children size


Diffs
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
 6731d1a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
 55a79ee 


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


Testing
---

unit tests in TestOwnerPrivileges passed


Thanks,

Na Li

Re: Review Request 68822: SENTRY-2371 Add a new thrift API for getting all privileges a user has

[Na Li via Review Board]

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




sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java
Lines 66 (patched)


Can you change the function from "list_sentry_privileges_for_user" to 
"list_sentry_privileges_by_user_and_itsgroups" to make it clear that it 
includes privileges directly assigned to this user and privileges through its 
groups?

You don't have to use "list_sentry_privileges_by_user_and_itsgroups" 
exactly, but use some name similar to it.



sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
Lines 416 (patched)


"and the group" to "and the groups" since a user can belong to multiple 
groups.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
Lines 113 (patched)


change the function name from "list-privileges-for-user" to the one you 
will use in sentry_policy_service.thrift



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 57 (patched)


It is better to add the new function in SentryStoreInterface instead of 
SentryStore as this class deals with the interface, not its implementation



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 915 (patched)


you have got groups in line 908 if the requestor is the same as 
principlaName. It should improve performance to reuse it under this situation



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
Line 227 (original), 229 (patched)


This is semi-public API. It is safer to add function than change the 
returned type of existing function. Inside implementation, you can let one 
function call another one to re-use code.



sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java
Lines 26 (patched)


If you only add new function in SentryStoreInterface, you can make it part 
of the TestSentryStore if those testing cases don't exsit.


- Na Li


On Sept. 24, 2018, 9:31 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> ---
> 
> (Updated Sept. 24, 2018, 9:31 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This commit adds a new thrift API list_sentry_privileges_for_user to List 
> sentry privileges granted to the given user and the group the user associated 
> with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java
>  0cbd8ab0a624d4c09aead4097f72762e12d1d21b 
>   
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
>  2e79e5646ae9102d8c0c28da4260a539254fcd15 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  236a07bdf5191cdc0f167f20a406b721b3dc506d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  3a9623b46f7c4335db18113574170f761da9a4ca 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1eda41b2b6bd940a404cc1ba09a861fe783ead04 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  0b4f4aa24bd3002c50bf4d80a6fa361f66052973 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java
>  

Re: Review Request 68822: SENTRY-2371 Add a new thrift API for getting all privileges a user has

[Na Li via Review Board]

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




sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
Lines 419 (patched)


I heard today that it is possible to assign user to a role in near future. 
So can you change the function name to 
"list_sentry_privileges_by_user_and_itsroles"? it will cover privileges

1) assigned to user directly
2) assigned to roles directly associated to a user
3) assigned to roles directly associated to groups that user belongs to.

Then when that feature of allowing user to be associated to a role is 
implemented, we don't need to change this API. And the intention of this 
function is to get all privileges for a user directly and indirectly



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
Lines 247 (patched)


Can you change the name of this function to 
"listSentryPrivilegesByUsersAndRoles" to be consistent with client API function 
name?


- Na Li


On Sept. 25, 2018, 9:23 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> ---
> 
> (Updated Sept. 25, 2018, 9:23 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This commit adds a new thrift API list_sentry_privileges_for_user to List 
> sentry privileges granted to the given user and the group the user associated 
> with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java
>  0cbd8ab0a624d4c09aead4097f72762e12d1d21b 
>   
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
>  2e79e5646ae9102d8c0c28da4260a539254fcd15 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  236a07bdf5191cdc0f167f20a406b721b3dc506d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  3a9623b46f7c4335db18113574170f761da9a4ca 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1eda41b2b6bd940a404cc1ba09a861fe783ead04 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  0b4f4aa24bd3002c50bf4d80a6fa361f66052973 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346 
> 
> 
> Diff: https://reviews.apache.org/r/68822/diff/3/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 68822: SENTRY-2371 Add a new thrift API for getting all privileges a user has

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Sept. 25, 2018, 9:23 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> ---
> 
> (Updated Sept. 25, 2018, 9:23 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This commit adds a new thrift API list_sentry_privileges_for_user to List 
> sentry privileges granted to the given user and the group the user associated 
> with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java
>  0cbd8ab0a624d4c09aead4097f72762e12d1d21b 
>   
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
>  2e79e5646ae9102d8c0c28da4260a539254fcd15 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  236a07bdf5191cdc0f167f20a406b721b3dc506d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  3a9623b46f7c4335db18113574170f761da9a4ca 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1eda41b2b6bd940a404cc1ba09a861fe783ead04 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  0b4f4aa24bd3002c50bf4d80a6fa361f66052973 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346 
> 
> 
> Diff: https://reviews.apache.org/r/68822/diff/3/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 68779: SENTRY-2409: ALTER TABLE SET OWNER does not allow to change the table if using only the table name

[Na Li via Review Board]

> On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
> > Lines 288-300 (patched)
> > 
> >
> > Is it possible to reuse the extractDatabase() and extractTable() 
> > commands that already does most of this split of db and table names?

extractDatabase() and extractTable() are deprecated, and we should not use them 
any more. I found we can use extractDbTableNameFromTOKTABLE(), which sets 
output DB and table. So I need to change alterTableSetOwnerPrivilege in 
HiveAuthzPrivilegesMap to use output db and table instead of input. 

In this way, it is more consistent with alterDbSetOwnerPrivilege, which also 
uses output db.


- Na


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


On Sept. 20, 2018, 4:24 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68779/
> ---
> 
> (Updated Sept. 20, 2018, 4:24 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2409
> https://issues.apache.org/jira/browse/sentry-2409
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> add code to get table name and DB name based on token children size
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  6731d1a 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
>  55a79ee 
> 
> 
> Diff: https://reviews.apache.org/r/68779/diff/1/
> 
> 
> Testing
> ---
> 
> unit tests in TestOwnerPrivileges passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 68838: SENTRY-2410: CREATE privileges on Hive does not allow a user to list all tables of a database

[Na Li via Review Board]

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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
Line 404 (original), 404 (patched)


create privilege on column does not make sense. 

Why the privileges on line 404 in DefaultSentryValidator.java is so 
different from line 645 in HiveAuthzBindingHookBase.java?


- Na Li


On Sept. 25, 2018, 1:41 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68838/
> ---
> 
> (Updated Sept. 25, 2018, 1:41 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2410
> https://issues.apache.org/jira/browse/SENTRY-2410
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The CREATE privilege is not allowing a user to see which tables already exist 
> in a database using the SHOW TABLES command. We should allow the SHOW TABLES 
> command to list all tables no matter the user who create them as there is no 
> case to avoid it.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
>  f0764767f32e4380a3f36b32e2e6a7420af11dde 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  da1956b8e7a3a7b3d1555c099f9424073c188ff2 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestShowMetadataPrivileges.java
>  88e697b32452de4af6ba6cc0b8c1a2bb7bdebeb6 
> 
> 
> Diff: https://reviews.apache.org/r/68838/diff/1/
> 
> 
> Testing
> ---
> 
> Made sure all the existing tests passed. Also updated some tests to cover 
> this change.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68831: SENTRY-2417: fix LocalGroupMappingService INI format class docs

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Sept. 24, 2018, 11:45 p.m., Dan Burkert wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68831/
> ---
> 
> (Updated Sept. 24, 2018, 11:45 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and Na Li.
> 
> 
> Bugs: SENTRY-2417
> https://issues.apache.org/jira/browse/SENTRY-2417
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> fix LocalGroupMappingService INI format class docs
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/LocalGroupMappingService.java
>  4430ce73b 
> 
> 
> Diff: https://reviews.apache.org/r/68831/diff/1/
> 
> 
> Testing
> ---
> 
> none - this is a doc only patch.
> 
> 
> Thanks,
> 
> Dan Burkert
> 
>

Re: Review Request 69353: SENTRY-2454: Add new sentry store api to gather the privileges for a list of authorizables.

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Dec. 18, 2018, 11:23 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69353/
> ---
> 
> (Updated Dec. 18, 2018, 11:23 p.m.)
> 
> 
> Review request for sentry and Sergio Pena.
> 
> 
> Bugs: SENTRY-2454
> https://issues.apache.org/jira/browse/SENTRY-2454
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> New sentry API should be implemented to fetch the privileges granted to 
> authorizables and it's children. authorizables include database, tables, 
> columns and URI's.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  63f53752f5a376015dce642ca1cb59aaa1dd16ba 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  85ea6d1c06c84f89108fb1313f505dba5e324eb3 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  b327e9e510483787311bf5218eac4039f04291ff 
> 
> 
> Diff: https://reviews.apache.org/r/69353/diff/4/
> 
> 
> Testing
> ---
> 
> Added new unit tests to test the API added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69501: SENTRY-2466: Create generic sentry store metrics

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Dec. 3, 2018, 5:22 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69501/
> ---
> 
> (Updated Dec. 3, 2018, 5:22 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2466
> https://issues.apache.org/jira/browse/SENTRY-2466
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now there are no metrics generated for table SENTRY_GM_PRIVILEGE. Thats 
> useful information not being displayed
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  214d78c53 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  e48eea377 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  8cea3398f 
> 
> 
> Diff: https://reviews.apache.org/r/69501/diff/1/
> 
> 
> Testing
> ---
> 
> $ mvn -f sentry-service/sentry-service-server/pom.xml test 
> -Dtest=TestSentryPolicyStoreProcessor
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69530: SENTRY-2476: Optimize deleting specific paths for objects

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3454 (original), 3451 (patched)


Why do you remove the input of "currentSnapshotID"? There could be multiple 
snapshots, and we need to make sure to remove the one in the latest Snapsho



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3455 (original), 3452 (patched)


if it possible that there are multiple instances in table AUTHZ_PATH, they 
are of the same value of "PATH_NAME", but belong to different  "authzObj"?

If that is the case, you delect more entries than you need to. Basically, 
you delete all entries matching input "paths" regardless the value of 
"authzObj". That is not right.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3551 (original), 3539 (patched)


why do you remove input "currentSnapshotID"



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3739 (patched)


path is a collection. Can you change the input parameter name to reflect 
that?


- Na Li


On Dec. 10, 2018, 2:11 a.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69530/
> ---
> 
> (Updated Dec. 10, 2018, 2:11 a.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2476
> https://issues.apache.org/jira/browse/SENTRY-2476
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now when we process a drop partition event, we fetch each path object 
> for paths_mapping object then find the one we want to delete and then delete 
> it. We should avoid fetching all objects and directly delete the path that 
> needs to be deleted
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac 
> 
> 
> Diff: https://reviews.apache.org/r/69530/diff/3/
> 
> 
> Testing
> ---
> 
> $ mvn -f sentry-service/sentry-service-server/pom.xml test 
> -Dtest=TestNotificationProcessor
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69530: SENTRY-2476: Optimize deleting specific paths for objects

[Na Li via Review Board]

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



Can you add more test cases to cover the cases that mutiple entries of same 
path_name, but different authzObj. And make sure you don't delete entries of 
different authzObj value.

- Na Li


On Dec. 10, 2018, 2:11 a.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69530/
> ---
> 
> (Updated Dec. 10, 2018, 2:11 a.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2476
> https://issues.apache.org/jira/browse/SENTRY-2476
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now when we process a drop partition event, we fetch each path object 
> for paths_mapping object then find the one we want to delete and then delete 
> it. We should avoid fetching all objects and directly delete the path that 
> needs to be deleted
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac 
> 
> 
> Diff: https://reviews.apache.org/r/69530/diff/3/
> 
> 
> Testing
> ---
> 
> $ mvn -f sentry-service/sentry-service-server/pom.xml test 
> -Dtest=TestNotificationProcessor
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69087: SENTRY-2249: Enable batch insert of HMS paths in Full Snapshot.

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Nov. 29, 2018, 10:14 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69087/
> ---
> 
> (Updated Nov. 29, 2018, 10:14 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2249
> https://issues.apache.org/jira/browse/SENTRY-2249
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently each entry in full snapshot of HMS is persisted one entry at a 
> time. Instead it could be optimized by persisting the path entries in 
> batches. DB operations are expensive, reducing the number of database 
> operations and around trip time will help. This would decrease the time to 
> persist the snapshot in to database significantly.
> 
> Size of the batch could be configurable.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  c51f25a0393b482814afcd3b7a19e547b689ac6e 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
>  b0eaff2120a80685da07c65a7706edf2be62ee01 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  e3ae24b0d11ec05537063e476a4a959bf2c43819 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  f5802d70145474c173fd4abfd2d2189e729e170c 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac6b285e286c12f7eec669b841cf76e9d 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  ca8c41610d3dcd12b267579c3d3cbe5b7ba7b208 
> 
> 
> Diff: https://reviews.apache.org/r/69087/diff/11/
> 
> 
> Testing
> ---
> 
> Made sure all the unit tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69536: SENTRY-2471: Table rename should sync Sentry privilege even without location information

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
Lines 418 (patched)


Can you make comment more explicit on path info is not complete?


- Na Li


On Dec. 10, 2018, 2:21 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69536/
> ---
> 
> (Updated Dec. 10, 2018, 2:21 a.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2471: Table rename should sync Sentry privilege even without location 
> information
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
>  7b7d0e1eb7ea1f17dea622e51ccc557e0b76fbff 
> 
> 
> Diff: https://reviews.apache.org/r/69536/diff/2/
> 
> 
> Testing
> ---
> 
> existing unit tests
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 69536: SENTRY-2471: Table rename should sync Sentry privilege even without location information

[Na Li via Review Board]

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



can you add test case to cover the issue you want to fix? There is no path 
info, but if the table name is changed, the privilege is moved to table of new 
name?

- Na Li


On Dec. 10, 2018, 2:21 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69536/
> ---
> 
> (Updated Dec. 10, 2018, 2:21 a.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2471: Table rename should sync Sentry privilege even without location 
> information
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
>  7b7d0e1eb7ea1f17dea622e51ccc557e0b76fbff 
> 
> 
> Diff: https://reviews.apache.org/r/69536/diff/2/
> 
> 
> Testing
> ---
> 
> existing unit tests
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 69586: SENTRY-2481: Filter HMS server-side objects based on HMS user authorization

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Dec. 19, 2018, 3:24 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69586/
> ---
> 
> (Updated Dec. 19, 2018, 3:24 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2481
> https://issues.apache.org/jira/browse/sentry-2481
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Re-use the SentryMetaStoreFilterHook to support HMS server-side object 
> filtering. The SentryMetaStoreFilterHook class was deprecated and not used in 
> the HMS client anymore (replaced by the calls to DefaultSentryValidator). Due 
> to code duplication between SentryMetaStoreFilterHook and 
> DefaultSentryValidator, a new class MetaStoreAuthzObjectFilter is created 
> that accepts different types of objects to be filtered (unit tests are added 
> to verify the cases).
> 
> 
> Diffs
> -
> 
>   .gitignore 6ce3a6c11f6caf743fb00271af2cb4d33a18aa5d 
>   pom.xml f28be5afb7c9673c0b111325d7728381f8c89d2f 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
>  520de52ac3a41d0b4c01b1bdf60944fd44add5e7 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivileges.java
>  c37ce646da97afb2e5c033fb3acf43190a4fae80 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  cd4ae4a8c80b34769c65d4b8b86b2d6ecc78b075 
>   sentry-binding/sentry-binding-hive/pom.xml 
> b74516d70eaf873ef46914e2fbcfe08753bc1be4 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
>  38ce2db374ee4f46190544479bc0713de2fce420 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/HiveAuthzBindingFactory.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  8ad9e50350a1a45ebdde9d8acb7f039b14a13f41 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
>  5ecc87f9be36d6096e30de1f3c8697cd2d4da091 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/authz/TestMetastoreAuthzObjectFilter.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentryMetaStoreFilterHook.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69586/diff/2/
> 
> 
> Testing
> ---
> 
> Added unit tests for the SentryMetaStoreFilterHook.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 69567: SENTRY-1679: HDFS tests configure MetastorePlugin which is gone

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Dec. 13, 2018, 9:10 p.m., Haley Reeve wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69567/
> ---
> 
> (Updated Dec. 13, 2018, 9:10 p.m.)
> 
> 
> Review request for sentry and Arjun Mishra.
> 
> 
> Bugs: SENTRY-1679
> https://issues.apache.org/jira/browse/SENTRY-1679
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In TestHDFSIntegrationBase#startHiveAndMetastore() there is:
> 
> hiveConf.set("sentry.metastore.plugins", 
> "org.apache.sentry.hdfs.MetastorePlugin");
> 
> But we no longer have MetastorePlugin, so this should be removed.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  3d7fbe3 
> 
> 
> Diff: https://reviews.apache.org/r/69567/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Haley Reeve
> 
>

Re: Review Request 69087: SENTRY-2249: Enable batch insert of HMS paths in Full Snapshot.

[Na Li via Review Board]

> On Nov. 29, 2018, 6:44 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
> > Lines 176 (patched)
> > 
> >
> > do you want to clear pathsToPersist after persisting them? Otherwise, 
> > next time it is called, those paths will be added again
> 
> kalyan kumar kalvagadda wrote:
> Persisting is the last thing that we do on an instance of 
> MAuthzPathsMapping. This shouldn't be an issue.

what happens if makePersistent is called, then more paths are added, then 
makePersistent is called again? If you don't clean pathsToPersist, the old 
paths (already persisted) will be persisted again. It is a bug, and need to be 
fixed.


- Na


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


On Nov. 29, 2018, 10:14 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69087/
> ---
> 
> (Updated Nov. 29, 2018, 10:14 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2249
> https://issues.apache.org/jira/browse/SENTRY-2249
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently each entry in full snapshot of HMS is persisted one entry at a 
> time. Instead it could be optimized by persisting the path entries in 
> batches. DB operations are expensive, reducing the number of database 
> operations and around trip time will help. This would decrease the time to 
> persist the snapshot in to database significantly.
> 
> Size of the batch could be configurable.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  c51f25a0393b482814afcd3b7a19e547b689ac6e 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
>  b0eaff2120a80685da07c65a7706edf2be62ee01 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  e3ae24b0d11ec05537063e476a4a959bf2c43819 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  f5802d70145474c173fd4abfd2d2189e729e170c 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac6b285e286c12f7eec669b841cf76e9d 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  ca8c41610d3dcd12b267579c3d3cbe5b7ba7b208 
> 
> 
> Diff: https://reviews.apache.org/r/69087/diff/11/
> 
> 
> Testing
> ---
> 
> Made sure all the unit tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69351: SENTRY-2458: Split web service from server service modules

[Na Li via Review Board]

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




sentry-dist/src/license/THIRD-PARTY.properties
Line 34 (original)


is this change intentially?



sentry-dist/src/license/THIRD-PARTY.properties
Line 37 (original), 37 (patched)


is this change intentially?


- Na Li


On Nov. 29, 2018, 7:58 p.m., Brian Towles wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69351/
> ---
> 
> (Updated Nov. 29, 2018, 7:58 p.m.)
> 
> 
> Review request for sentry, Anthony Young-Garner, kalyan kumar kalvagadda, Na 
> Li, Steve Moist, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2458
> https://issues.apache.org/jira/browse/SENTRY-2458
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2458: Split web service from server service modules
> 
> In order to additional modules to be added to Sentry there needs to be a 
> separation of some of the features used by service-server into other modules.
> 
> This will allow the web server to be pulled out from the base server and 
> allow for additional modules to be able to add web services and functionality 
> to the web interface without depending up the whole server module.
> 
> It will use the SPI to dynamically include servlets and content from other 
> modules.
> 
> It creates a sentry-service-providers to allow for Server and sub modules SPI 
> providers to be defined externally and not depended on directly.
> 
> 
> Diffs
> -
> 
>   pom.xml 46ca38e9a 
>   sentry-dist/pom.xml 62558d2e0 
>   sentry-dist/src/license/THIRD-PARTY.properties a1084db69 
>   sentry-dist/src/main/assembly/bin.xml 986530c55 
>   sentry-provider/sentry-provider-db/pom.xml df569474a 
>   sentry-service/pom.xml e653189eb 
>   sentry-service/sentry-service-providers/pom.xml PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/AttributeDesc.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/FilterDesc.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/ServletDesc.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/WebServiceProvider.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/WebServiceProviderFactory.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/WebServiceSpi.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/resources/META-INF/services/org.apache.sentry.spi.Spi
>  PRE-CREATION 
>   sentry-service/sentry-service-server/pom.xml 44540ad5d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/ConfServlet.java
>  862548745 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/LogLevelServlet.java
>  af81d6fce 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/PubSubServlet.java
>  8da35f10f 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/RolesServlet.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryAdminServlet.java
>  5dc6cd6c4 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryAuthFilter.java
>  23121ecf5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryServiceWebServiceProvider.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryWebServer.java
>  befe6c3ed 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  b3a4934df 
>   
> sentry-service/sentry-service-server/src/main/resources/META-INF/services/org.apache.sentry.server.provider.webservice.WebServiceProviderFactory
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/webapp/css/bootstrap-theme.min.css
>  c31428b07 
>   sentry-service/sentry-service-server/src/main/webapp/css/bootstrap.min.css 
> a553c4f5e 
>   sentry-service/sentry-service-server/src/main/webapp/css/sentry.css  
>   sentry-service/sentry-service-server/src/main/webapp/sentry.png  
>   
> 

Re: Review Request 69087: SENTRY-2249: Enable batch insert of HMS paths in Full Snapshot.

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Line 78 (original), 88 (patched)


this seems to be a bug. If paths contains path_1, and caller adds path_2, 
this change results in paths contains only path_2, but lost path_1.


- Na Li


On Nov. 21, 2018, 7:48 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69087/
> ---
> 
> (Updated Nov. 21, 2018, 7:48 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2249
> https://issues.apache.org/jira/browse/SENTRY-2249
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently each entry in full snapshot of HMS is persisted one entry at a 
> time. Instead it could be optimized by persisting the path entries in 
> batches. DB operations are expensive, reducing the number of database 
> operations and around trip time will help. This would decrease the time to 
> persist the snapshot in to database significantly.
> 
> Size of the batch could be configurable.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  c51f25a0393b482814afcd3b7a19e547b689ac6e 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
>  b0eaff2120a80685da07c65a7706edf2be62ee01 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  e3ae24b0d11ec05537063e476a4a959bf2c43819 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
>  ab262d0a849852bd95d88dd099dc6bf187715cad 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  f5802d70145474c173fd4abfd2d2189e729e170c 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac6b285e286c12f7eec669b841cf76e9d 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  ca8c41610d3dcd12b267579c3d3cbe5b7ba7b208 
> 
> 
> Diff: https://reviews.apache.org/r/69087/diff/8/
> 
> 
> Testing
> ---
> 
> Made sure all the unit tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69087: SENTRY-2249: Enable batch insert of HMS paths in Full Snapshot.

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Lines 176 (patched)


do you want to clear pathsToPersist after persisting them? Otherwise, next 
time it is called, those paths will be added again


- Na Li


On Nov. 29, 2018, 6:32 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69087/
> ---
> 
> (Updated Nov. 29, 2018, 6:32 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2249
> https://issues.apache.org/jira/browse/SENTRY-2249
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently each entry in full snapshot of HMS is persisted one entry at a 
> time. Instead it could be optimized by persisting the path entries in 
> batches. DB operations are expensive, reducing the number of database 
> operations and around trip time will help. This would decrease the time to 
> persist the snapshot in to database significantly.
> 
> Size of the batch could be configurable.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  c51f25a0393b482814afcd3b7a19e547b689ac6e 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
>  b0eaff2120a80685da07c65a7706edf2be62ee01 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  e3ae24b0d11ec05537063e476a4a959bf2c43819 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  f5802d70145474c173fd4abfd2d2189e729e170c 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac6b285e286c12f7eec669b841cf76e9d 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  ca8c41610d3dcd12b267579c3d3cbe5b7ba7b208 
> 
> 
> Diff: https://reviews.apache.org/r/69087/diff/10/
> 
> 
> Testing
> ---
> 
> Made sure all the unit tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69352: SENTRY-2452: Change the thrift interface to send the list of authorizable to sentry server

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Dec. 7, 2018, 3:14 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69352/
> ---
> 
> (Updated Dec. 7, 2018, 3:14 p.m.)
> 
> 
> Review request for sentry and Sergio Pena.
> 
> 
> Bugs: SENTRY-2452
> https://issues.apache.org/jira/browse/SENTRY-2452
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> TSentryExportMappingDataRequest and TSentryImportMappingDataRequest which are 
> used to send export and import requests to sentry server should be changed to 
> be able to send a list authorizables for which permission information has to 
> be exported/imported.
> 
> These requests should accommodate source and target cluster information as 
> well.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegePrincipalType.java
>  6eb85217b9229438b9adbd1546a408dc507f1645 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryExportMappingDataRequest.java
>  13e57e04dd779825e2986b1e527f4e556271a162 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryImportMappingDataRequest.java
>  21f3bdf5e304fe2f49684c999d0aa0e0362320de 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryPrivilegesResponse.java
>  cea868f0524c25bf63cfb7c532829354a280df28 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  83393a98df5e10b324f74dc12f10c20bc5f02651 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  68d864cfbdf18057d87a65a04af8991292aadccf 
>   
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
>  3364648dc88acbb8de1cc925fe8c512f34a4b064 
>   
> sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
>  2dc0975f482f760c18c438c1418630a7ef6a4cbb 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  b9e3bf2921a0696da639e1b1bee5d83cf2b9cee0 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryServiceImportExport.java
>  cf1fdab382034c8950da2613ef9b0cf1af912e33 
> 
> 
> Diff: https://reviews.apache.org/r/69352/diff/5/
> 
> 
> Testing
> ---
> 
> Made sure that existiung tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69353: SENTRY-2454: Add new sentrys tore api to gather the privileges for a list of authorizables.

[Na Li via Review Board]

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



typo: "sentrys tore" should be "sentry store"

- Na Li


On Dec. 7, 2018, 12:05 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69353/
> ---
> 
> (Updated Dec. 7, 2018, 12:05 a.m.)
> 
> 
> Review request for sentry and Sergio Pena.
> 
> 
> Bugs: SENTRY-2454
> https://issues.apache.org/jira/browse/SENTRY-2454
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> New sentry API should be implemented to fetch the privileges granted to 
> authorizables and it's children. authorizables include database, tables, 
> columns and URI's.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac6b285e286c12f7eec669b841cf76e9d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  e48eea377b842475f72b6fab4567a82c8fd93098 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  ca8c41610d3dcd12b267579c3d3cbe5b7ba7b208 
> 
> 
> Diff: https://reviews.apache.org/r/69353/diff/2/
> 
> 
> Testing
> ---
> 
> Added new unit tests to test the API added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69353: SENTRY-2454: Add new sentrys tore api to gather the privileges for a list of authorizables.

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3932 (patched)


If column name is not null, should add that in filter



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3943 (patched)


You don't fetch users of that privilege. Do you want to return that? If the 
caller does not want it, it can be dropped. Or let a input indicating if we 
want to get users as well


- Na Li


On Dec. 7, 2018, 12:05 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69353/
> ---
> 
> (Updated Dec. 7, 2018, 12:05 a.m.)
> 
> 
> Review request for sentry and Sergio Pena.
> 
> 
> Bugs: SENTRY-2454
> https://issues.apache.org/jira/browse/SENTRY-2454
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> New sentry API should be implemented to fetch the privileges granted to 
> authorizables and it's children. authorizables include database, tables, 
> columns and URI's.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac6b285e286c12f7eec669b841cf76e9d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  e48eea377b842475f72b6fab4567a82c8fd93098 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  ca8c41610d3dcd12b267579c3d3cbe5b7ba7b208 
> 
> 
> Diff: https://reviews.apache.org/r/69353/diff/2/
> 
> 
> Testing
> ---
> 
> Added new unit tests to test the API added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69448: SENTRY-2464: Catch exception thrown on first reload for UpdatableCache

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Nov. 27, 2018, 9:34 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69448/
> ---
> 
> (Updated Nov. 27, 2018, 9:34 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2464
> https://issues.apache.org/jira/browse/SENTRY-2464
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> On starting UpdatableCache update thread, if blockUntilFirstReload value is 
> set to true, an exception thrownwill cause the cache to never initialize and 
> services using this cache will stop
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
>  0dd7b4a61 
> 
> 
> Diff: https://reviews.apache.org/r/69448/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69448: SENTRY-2464: Catch exception thrown on first reload for UpdatableCache

[Na Li via Review Board]

> On Nov. 26, 2018, 7:44 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
> > Line 37 (original), 37 (patched)
> > 
> >
> > should this be volatile  or atomic?
> 
> Arjun Mishra wrote:
> Lina, can you explain why adding volatile here is needed? For each Kafka 
> broker there is a single KafkaAuthzBinding instance. So 
> UpdatableCache#startUpdateThread will be called once by a KafkaBroker. This 
> means there will be a reloadData thread for each Kafka broker.

Is it possible for more than one threads to access that single 
KafkaAuthzBinding instance? If so, we should define "initialized" as volatile 
to handle concurrency correctly. Otherwise, you don't need to do so


- Na


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


On Nov. 26, 2018, 7:36 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69448/
> ---
> 
> (Updated Nov. 26, 2018, 7:36 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2464
> https://issues.apache.org/jira/browse/SENTRY-2464
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> On starting UpdatableCache update thread, if blockUntilFirstReload value is 
> set to true, an exception thrownwill cause the cache to never initialize and 
> services using this cache will stop
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
>  0dd7b4a61 
> 
> 
> Diff: https://reviews.apache.org/r/69448/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69087: SENTRY-2249: Enable batch insert of HMS paths in Full Snapshot.

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3412 (original), 3430 (patched)


right now, only one thead from leader is calling this function, so 
assigning AuthzObjectID in this way is OK. 

If in the future, multiple threads could call this function at the same 
time, will we have key conflict exception because multiple entries with same ID 
are inserted to DB?


- Na Li


On Nov. 20, 2018, 12:22 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69087/
> ---
> 
> (Updated Nov. 20, 2018, 12:22 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2249
> https://issues.apache.org/jira/browse/SENTRY-2249
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently each entry in full snapshot of HMS is persisted one entry at a 
> time. Instead it could be optimized by persisting the path entries in 
> batches. DB operations are expensive, reducing the number of database 
> operations and around trip time will help. This would decrease the time to 
> persist the snapshot in to database significantly.
> 
> Size of the batch could be configurable.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  c51f25a0393b482814afcd3b7a19e547b689ac6e 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
>  b0eaff2120a80685da07c65a7706edf2be62ee01 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  e3ae24b0d11ec05537063e476a4a959bf2c43819 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac6b285e286c12f7eec669b841cf76e9d 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  ca8c41610d3dcd12b267579c3d3cbe5b7ba7b208 
> 
> 
> Diff: https://reviews.apache.org/r/69087/diff/7/
> 
> 
> Testing
> ---
> 
> Made sure all the unit tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69087: SENTRY-2249: Enable batch insert of HMS paths in Full Snapshot.

[Na Li via Review Board]

> On Nov. 20, 2018, 5:08 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
> > Lines 179 (patched)
> > 
> >
> > can you set the filter that path can be one of the paths, so a single 
> > query can delete multiple paths?

You can use similar approach as QueryParamBuilder.addRolesFilter(), so a single 
query can delete multiple paths. And you can limit the number of filter entries 
to avoid too big query.


- Na


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


On Nov. 20, 2018, 12:22 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69087/
> ---
> 
> (Updated Nov. 20, 2018, 12:22 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2249
> https://issues.apache.org/jira/browse/SENTRY-2249
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently each entry in full snapshot of HMS is persisted one entry at a 
> time. Instead it could be optimized by persisting the path entries in 
> batches. DB operations are expensive, reducing the number of database 
> operations and around trip time will help. This would decrease the time to 
> persist the snapshot in to database significantly.
> 
> Size of the batch could be configurable.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  c51f25a0393b482814afcd3b7a19e547b689ac6e 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
>  b0eaff2120a80685da07c65a7706edf2be62ee01 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  e3ae24b0d11ec05537063e476a4a959bf2c43819 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac6b285e286c12f7eec669b841cf76e9d 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  ca8c41610d3dcd12b267579c3d3cbe5b7ba7b208 
> 
> 
> Diff: https://reviews.apache.org/r/69087/diff/7/
> 
> 
> Testing
> ---
> 
> Made sure all the unit tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69087: SENTRY-2249: Enable batch insert of HMS paths in Full Snapshot.

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3457 (original), 3471 (patched)


why not call mAuthzPathsMapping.deletePersistent(pm, paths);?

So we only need to delete all paths with one call instead of loop through 
all paths and delete one path at one time.


- Na Li


On Nov. 20, 2018, 12:22 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69087/
> ---
> 
> (Updated Nov. 20, 2018, 12:22 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2249
> https://issues.apache.org/jira/browse/SENTRY-2249
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently each entry in full snapshot of HMS is persisted one entry at a 
> time. Instead it could be optimized by persisting the path entries in 
> batches. DB operations are expensive, reducing the number of database 
> operations and around trip time will help. This would decrease the time to 
> persist the snapshot in to database significantly.
> 
> Size of the batch could be configurable.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  c51f25a0393b482814afcd3b7a19e547b689ac6e 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
>  b0eaff2120a80685da07c65a7706edf2be62ee01 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  e3ae24b0d11ec05537063e476a4a959bf2c43819 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac6b285e286c12f7eec669b841cf76e9d 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  ca8c41610d3dcd12b267579c3d3cbe5b7ba7b208 
> 
> 
> Diff: https://reviews.apache.org/r/69087/diff/7/
> 
> 
> Testing
> ---
> 
> Made sure all the unit tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69087: SENTRY-2249: Enable batch insert of HMS paths in Full Snapshot.

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3699 (original), 3700 (patched)


Could this way of allocating ID handle multiple threads without exception?


- Na Li


On Nov. 20, 2018, 12:22 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69087/
> ---
> 
> (Updated Nov. 20, 2018, 12:22 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2249
> https://issues.apache.org/jira/browse/SENTRY-2249
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently each entry in full snapshot of HMS is persisted one entry at a 
> time. Instead it could be optimized by persisting the path entries in 
> batches. DB operations are expensive, reducing the number of database 
> operations and around trip time will help. This would decrease the time to 
> persist the snapshot in to database significantly.
> 
> Size of the batch could be configurable.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  c51f25a0393b482814afcd3b7a19e547b689ac6e 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
>  b0eaff2120a80685da07c65a7706edf2be62ee01 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  e3ae24b0d11ec05537063e476a4a959bf2c43819 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac6b285e286c12f7eec669b841cf76e9d 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  ca8c41610d3dcd12b267579c3d3cbe5b7ba7b208 
> 
> 
> Diff: https://reviews.apache.org/r/69087/diff/7/
> 
> 
> Testing
> ---
> 
> Made sure all the unit tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69415: SENTRY-2463: Revoking ALL or * should revoke any other privilege on the entity

[Na Li via Review Board]

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




sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java
Lines 2303 (patched)


you should test "insert", "create" as well



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java
Lines 2311 (patched)


You don't need to close statement and connection for each command, and then 
created for the same user. You can do it only at the end of the test.



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java
Lines 2350 (patched)


test "insert" and "create"


- Na Li


On Nov. 20, 2018, 7:47 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69415/
> ---
> 
> (Updated Nov. 20, 2018, 7:47 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2463
> https://issues.apache.org/jira/browse/SENTRY-2463
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now if we initially grant ALL and revoke SELECT or INSERT, the 
> privilege gets "modified" to INSERT or SELECT. However, conversely if we 
> initially grant SELECT or INSERT and revoke ALL, no privileges are dropped
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java
>  6a7c1f392 
> 
> 
> Diff: https://reviews.apache.org/r/69415/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-tests/sentry-tests-hive/pom.xml test -Dtest=TestDatabaseProvider
> mvn -f sentry-service/sentry-service-server/pom.xml test 
> -Dtest=TestSentryStore
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69351: SENTRY-2458: Split web service from server service modules

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Nov. 27, 2018, 3:32 a.m., Brian Towles wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69351/
> ---
> 
> (Updated Nov. 27, 2018, 3:32 a.m.)
> 
> 
> Review request for sentry, Anthony Young-Garner, kalyan kumar kalvagadda, Na 
> Li, Steve Moist, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2458
> https://issues.apache.org/jira/browse/SENTRY-2458
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2458: Split web service from server service modules
> 
> In order to additional modules to be added to Sentry there needs to be a 
> separation of some of the features used by service-server into other modules.
> 
> This will allow the web server to be pulled out from the base server and 
> allow for additional modules to be able to add web services and functionality 
> to the web interface without depending up the whole server module.
> 
> It will use the SPI to dynamically include servlets and content from other 
> modules.
> 
> It creates a sentry-service-providers to allow for Server and sub modules SPI 
> providers to be defined externally and not depended on directly.
> 
> 
> Diffs
> -
> 
>   pom.xml 46ca38e9a 
>   sentry-dist/pom.xml 62558d2e0 
>   sentry-dist/src/main/assembly/bin.xml 986530c55 
>   sentry-provider/sentry-provider-db/pom.xml df569474a 
>   sentry-service/pom.xml e653189eb 
>   sentry-service/sentry-service-providers/pom.xml PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/AttributeDesc.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/FilterDesc.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/ServletDesc.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/WebServiceProvider.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/WebServiceProviderFactory.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/WebServiceSpi.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/resources/META-INF/services/org.apache.sentry.spi.Spi
>  PRE-CREATION 
>   sentry-service/sentry-service-server/pom.xml 44540ad5d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/ConfServlet.java
>  862548745 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/LogLevelServlet.java
>  af81d6fce 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/PubSubServlet.java
>  8da35f10f 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/RolesServlet.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryAdminServlet.java
>  5dc6cd6c4 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryAuthFilter.java
>  23121ecf5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryServiceWebServiceProvider.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryWebServer.java
>  befe6c3ed 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  b3a4934df 
>   
> sentry-service/sentry-service-server/src/main/resources/META-INF/services/org.apache.sentry.server.provider.webservice.WebServiceProviderFactory
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/webapp/css/bootstrap-theme.min.css
>  c31428b07 
>   sentry-service/sentry-service-server/src/main/webapp/css/bootstrap.min.css 
> a553c4f5e 
>   sentry-service/sentry-service-server/src/main/webapp/css/sentry.css  
>   sentry-service/sentry-service-server/src/main/webapp/sentry.png  
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryWebServerWithoutSecurity.java
>  6e741e895 
>   sentry-service/sentry-service-web/pom.xml PRE-CREATION 
>   
> sentry-service/sentry-service-web/src/main/java/org/apache/sentry/service/web/DefaultWebServicesProvider.java
>  PRE-CREATION 
>   
> 

Re: Review Request 69448: SENTRY-2464: Catch exception thrown on first reload for UpdatableCache

[Na Li via Review Board]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
Line 37 (original), 37 (patched)


should this be volatile  or atomic?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
Line 145 (original), 149 (patched)


It is better to make initialized volatile


- Na Li


On Nov. 26, 2018, 7:36 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69448/
> ---
> 
> (Updated Nov. 26, 2018, 7:36 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2464
> https://issues.apache.org/jira/browse/SENTRY-2464
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> On starting UpdatableCache update thread, if blockUntilFirstReload value is 
> set to true, an exception thrownwill cause the cache to never initialize and 
> services using this cache will stop
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
>  0dd7b4a61 
> 
> 
> Diff: https://reviews.apache.org/r/69448/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69351: SENTRY-2458: Split web service from server service modules

[Na Li via Review Board]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/ConfServlet.java
Lines 20 (patched)


or you should move it from under "sentry-service/sentry-service-server" to 
"sentry-service/sentry-service-web"


- Na Li


On Nov. 20, 2018, 7:30 p.m., Brian Towles wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69351/
> ---
> 
> (Updated Nov. 20, 2018, 7:30 p.m.)
> 
> 
> Review request for sentry, Anthony Young-Garner, kalyan kumar kalvagadda, Na 
> Li, Steve Moist, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2458
> https://issues.apache.org/jira/browse/SENTRY-2458
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2458: Split web service from server service modules
> 
> In order to additional modules to be added to Sentry there needs to be a 
> separation of some of the features used by service-server into other modules.
> 
> This will allow the web server to be pulled out from the base server and 
> allow for additional modules to be able to add web services and functionality 
> to the web interface without depending up the whole server module.
> 
> It will use the SPI to dynamically include servlets and content from other 
> modules.
> 
> It creates a sentry-service-providers to allow for Server and sub modules SPI 
> providers to be defined externally and not depended on directly.
> 
> 
> Diffs
> -
> 
>   pom.xml 46ca38e9a 
>   sentry-dist/pom.xml 62558d2e0 
>   sentry-dist/src/main/assembly/bin.xml 986530c55 
>   sentry-provider/sentry-provider-db/pom.xml df569474a 
>   sentry-service/pom.xml e653189eb 
>   sentry-service/sentry-service-providers/pom.xml PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/AttributeDesc.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/FilterDesc.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/SentryWebServiceProvider.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/SentryWebServiceProviderFactory.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/SentryWebServiceSpi.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/ServletDesc.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/resources/META-INF/services/org.apache.sentry.spi.Spi
>  PRE-CREATION 
>   sentry-service/sentry-service-server/pom.xml 44540ad5d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/ConfServlet.java
>  862548745 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/LogLevelServlet.java
>  af81d6fce 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/PubSubServlet.java
>  8da35f10f 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/RolesServlet.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryAdminServlet.java
>  5dc6cd6c4 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryAuthFilter.java
>  23121ecf5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryServiceWebServiceProvider.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryWebServer.java
>  befe6c3ed 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  b3a4934df 
>   
> sentry-service/sentry-service-server/src/main/resources/META-INF/services/org.apache.sentry.server.provider.webservice.SentryWebServiceProviderFactory
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/webapp/css/bootstrap-theme.min.css
>  c31428b07 
>   sentry-service/sentry-service-server/src/main/webapp/css/bootstrap.min.css 
> a553c4f5e 
>   sentry-service/sentry-service-server/src/main/webapp/css/sentry.css  
>   sentry-service/sentry-service-server/src/main/webapp/sentry.png  
>   
> 

Re: Review Request 69351: SENTRY-2458: Split web service from server service modules

[Na Li via Review Board]

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




sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/SentryWebServiceProvider.java
Lines 30 (patched)


This is interface. Should we follow the convention and call it 
"SentryWebServiceProviderInterface"? And its implementation as 
"SentryWebServiceProvider"?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/ConfServlet.java
Lines 20 (patched)


should this be the same package as "RolesServlet.java"?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/RolesServlet.java
Lines 49 (patched)


instead of creating SentryStore directly, it would be better to use 
approach similar in 
https://github.com/apache/sentry/blob/master/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryService.java#L187



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryServiceWebServiceProvider.java
Lines 40 (patched)


It is not easy to tell that SentryServiceWebServiceProvider is 
implementation, and  SentryWebServiceProvider is interface. 

It is easier to read code if we call "SentryWebServiceProvider" as 
"SentryWebServiceProviderInterface" and "SentryServiceWebServiceProvider " as 
"SentryWebServiceProvider".



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryServiceWebServiceProvider.java
Lines 64 (patched)


it returns the same result as /admi/roles. Should we remove this line?


- Na Li


On Nov. 20, 2018, 7:30 p.m., Brian Towles wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69351/
> ---
> 
> (Updated Nov. 20, 2018, 7:30 p.m.)
> 
> 
> Review request for sentry, Anthony Young-Garner, kalyan kumar kalvagadda, Na 
> Li, Steve Moist, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2458
> https://issues.apache.org/jira/browse/SENTRY-2458
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2458: Split web service from server service modules
> 
> In order to additional modules to be added to Sentry there needs to be a 
> separation of some of the features used by service-server into other modules.
> 
> This will allow the web server to be pulled out from the base server and 
> allow for additional modules to be able to add web services and functionality 
> to the web interface without depending up the whole server module.
> 
> It will use the SPI to dynamically include servlets and content from other 
> modules.
> 
> It creates a sentry-service-providers to allow for Server and sub modules SPI 
> providers to be defined externally and not depended on directly.
> 
> 
> Diffs
> -
> 
>   pom.xml 46ca38e9a 
>   sentry-dist/pom.xml 62558d2e0 
>   sentry-dist/src/main/assembly/bin.xml 986530c55 
>   sentry-provider/sentry-provider-db/pom.xml df569474a 
>   sentry-service/pom.xml e653189eb 
>   sentry-service/sentry-service-providers/pom.xml PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/AttributeDesc.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/FilterDesc.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/SentryWebServiceProvider.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/SentryWebServiceProviderFactory.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/SentryWebServiceSpi.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/java/org/apache/sentry/server/provider/webservice/ServletDesc.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-providers/src/main/resources/META-INF/services/org.apache.sentry.spi.Spi
>  PRE-CREATION 
>   sentry-service/sentry-service-server/pom.xml 44540ad5d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/ConfServlet.java
>  862548745 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/LogLevelServlet.java
>  af81d6fce 
>   
> 

Re: Review Request 69702: SENTRY-2483: Implement HMS PreReadEvent support in MetastoreAuthzBinding

[Na Li via Review Board]

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

(Updated Jan. 9, 2019, 10:39 p.m.)


Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.


Bugs: sentry-2483
https://issues.apache.org/jira/browse/sentry-2483


Repository: sentry


Description
---

Add READ_DATABASE and READ_TABLE events support to provide read authorization 
to HMS.

This is based on changed made by Sergio at https://reviews.apache.org/r/69620/, 
and add code to fix unstable e2e tests


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 328d2b5 
  sentry-tests/sentry-tests-hive/pom.xml 74777bb 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 47f7466 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationTogglingConf.java
 e504a8a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 8bf486e 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
 7d41348 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
 3c28fd0 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 f8f304f 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
 9fa42f2 


Diff: https://reviews.apache.org/r/69702/diff/2/

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


Testing
---

add new e2e tests for READ_DATABASE and READ_TABLE at HMS


Thanks,

Na Li

Re: Review Request 69620: SENTRY-2483: Implement HMS PreReadEvent support in MetastoreAuthzBinding

[Na Li via Review Board]

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




sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
Lines 284 (patched)


if enableAuthorizingObjectStore is false, should you set 
METASTORE_RAW_STORE_IMPL to be ObjectStore? The default value is 
"org.apache.hadoop.hive.metastore.ObjectStore". Setting it here would make it 
more obvious what implementation is used.


- Na Li


On Dec. 21, 2018, 5:39 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69620/
> ---
> 
> (Updated Dec. 21, 2018, 5:39 p.m.)
> 
> 
> Review request for sentry and Na Li.
> 
> 
> Bugs: sentry-2483
> https://issues.apache.org/jira/browse/sentry-2483
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add READ_DATABASE and READ_TABLE events support to provide read authorization 
> to HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  328d2b5c69451922e062cc3f04d37c5e7347d17f 
>   sentry-tests/sentry-tests-hive/pom.xml 
> 74777bbff590ea63c18492c77ae86042734d8e70 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
>  8bf486e7d7d7a2e89278f1287115bf835513ef3f 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
>  7d41348572f0c01001b6bfa03d5ffb780f5a5e75 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
>  f8f304fbb9926d98939ee0aa8c74f0abc8789fa9 
> 
> 
> Diff: https://reviews.apache.org/r/69620/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>