Re: Review Request 71039: SENTRY-2528: Format exception when fetching a full snapshot

[Arjun Mishra via Review Board]

> On July 9, 2019, 6:39 p.m., kalyan kumar kalvagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
> > Line 548 (original), 548 (patched)
> > 
> >
> > Consider changing it as below
> > 
> > percentageDatabasesFetched = totalNumberOfDatabases > 0? 
> > ((totalNumberOfDatabasesFetched*100)/totalNumberOfDatabases):0;
> > 
> > 
> > This way you can avaoid the explicit casting. This applies below 
> > statements as well.

We need to cast else long/long will be a long and the decimal value will be 
dropped


- Arjun


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


On July 9, 2019, 3 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71039/
> ---
> 
> (Updated July 9, 2019, 3 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: SENTRY-2528
> https://issues.apache.org/jira/browse/SENTRY-2528
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When fetching a full snapshot we get the below error. This is a regression of 
> SENTRY-2301
> 
> 2019-07-07 23:07:39,677 ERROR 
> org.apache.sentry.service.thrift.SentryHMSClient: Snapshot created failed
> java.util.IllegalFormatConversionException: f != java.lang.Long
> at 
> java.util.Formatter$FormatSpecifier.failConversion(Formatter.java:4302)
> at java.util.Formatter$FormatSpecifier.printFloat(Formatter.java:2806)
> at java.util.Formatter$FormatSpecifier.print(Formatter.java:2753)
> at java.util.Formatter.format(Formatter.java:2520)
> at java.util.Formatter.format(Formatter.java:2455)
> at java.lang.String.format(String.java:2940)
> at 
> org.apache.sentry.service.thrift.FullUpdateInitializer.getFullHMSSnapshot(FullUpdateInitializer.java:552)
> at 
> org.apache.sentry.service.thrift.SentryHMSClient.fetchFullUpdate(SentryHMSClient.java:244)
> at 
> org.apache.sentry.service.thrift.SentryHMSClient.getFullSnapshot(SentryHMSClient.java:147)
> at 
> org.apache.sentry.provider.db.service.persistent.HMSFollower.createFullSnapshot(HMSFollower.java:409)
> at 
> org.apache.sentry.provider.db.service.persistent.HMSFollower.syncupWithHms(HMSFollower.java:237)
> at 
> org.apache.sentry.provider.db.service.persistent.HMSFollower.run(HMSFollower.java:198)
> at 
> java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
> at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308)
> at 
> java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:180)
> at 
> java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:294)
> at 
> java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
> at 
> java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
> at java.lang.Thread.run(Thread.java:748)
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  d4bca42faea51d87164568c23bd7849c56450f32 
> 
> 
> Diff: https://reviews.apache.org/r/71039/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 71039: SENTRY-2528: Format exception when fetching a full snapshot

[Arjun Mishra via Review Board]

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


Ship it!




Ship It!

- Arjun Mishra


On July 9, 2019, 3 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71039/
> ---
> 
> (Updated July 9, 2019, 3 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: SENTRY-2528
> https://issues.apache.org/jira/browse/SENTRY-2528
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When fetching a full snapshot we get the below error. This is a regression of 
> SENTRY-2301
> 
> 2019-07-07 23:07:39,677 ERROR 
> org.apache.sentry.service.thrift.SentryHMSClient: Snapshot created failed
> java.util.IllegalFormatConversionException: f != java.lang.Long
> at 
> java.util.Formatter$FormatSpecifier.failConversion(Formatter.java:4302)
> at java.util.Formatter$FormatSpecifier.printFloat(Formatter.java:2806)
> at java.util.Formatter$FormatSpecifier.print(Formatter.java:2753)
> at java.util.Formatter.format(Formatter.java:2520)
> at java.util.Formatter.format(Formatter.java:2455)
> at java.lang.String.format(String.java:2940)
> at 
> org.apache.sentry.service.thrift.FullUpdateInitializer.getFullHMSSnapshot(FullUpdateInitializer.java:552)
> at 
> org.apache.sentry.service.thrift.SentryHMSClient.fetchFullUpdate(SentryHMSClient.java:244)
> at 
> org.apache.sentry.service.thrift.SentryHMSClient.getFullSnapshot(SentryHMSClient.java:147)
> at 
> org.apache.sentry.provider.db.service.persistent.HMSFollower.createFullSnapshot(HMSFollower.java:409)
> at 
> org.apache.sentry.provider.db.service.persistent.HMSFollower.syncupWithHms(HMSFollower.java:237)
> at 
> org.apache.sentry.provider.db.service.persistent.HMSFollower.run(HMSFollower.java:198)
> at 
> java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
> at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308)
> at 
> java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:180)
> at 
> java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:294)
> at 
> java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
> at 
> java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
> at java.lang.Thread.run(Thread.java:748)
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  d4bca42faea51d87164568c23bd7849c56450f32 
> 
> 
> Diff: https://reviews.apache.org/r/71039/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Review Request 71039: SENTRY-2528: Format exception when fetching a full snapshot

[Arjun Mishra via Review Board]

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

Review request for sentry and kalyan kumar kalvagadda.


Bugs: SENTRY-2528
https://issues.apache.org/jira/browse/SENTRY-2528


Repository: sentry


Description
---

When fetching a full snapshot we get the below error. This is a regression of 
SENTRY-2301

2019-07-07 23:07:39,677 ERROR org.apache.sentry.service.thrift.SentryHMSClient: 
Snapshot created failed
java.util.IllegalFormatConversionException: f != java.lang.Long
at 
java.util.Formatter$FormatSpecifier.failConversion(Formatter.java:4302)
at java.util.Formatter$FormatSpecifier.printFloat(Formatter.java:2806)
at java.util.Formatter$FormatSpecifier.print(Formatter.java:2753)
at java.util.Formatter.format(Formatter.java:2520)
at java.util.Formatter.format(Formatter.java:2455)
at java.lang.String.format(String.java:2940)
at 
org.apache.sentry.service.thrift.FullUpdateInitializer.getFullHMSSnapshot(FullUpdateInitializer.java:552)
at 
org.apache.sentry.service.thrift.SentryHMSClient.fetchFullUpdate(SentryHMSClient.java:244)
at 
org.apache.sentry.service.thrift.SentryHMSClient.getFullSnapshot(SentryHMSClient.java:147)
at 
org.apache.sentry.provider.db.service.persistent.HMSFollower.createFullSnapshot(HMSFollower.java:409)
at 
org.apache.sentry.provider.db.service.persistent.HMSFollower.syncupWithHms(HMSFollower.java:237)
at 
org.apache.sentry.provider.db.service.persistent.HMSFollower.run(HMSFollower.java:198)
at 
java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308)
at 
java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:180)
at 
java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:294)
at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:748)


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
 d4bca42faea51d87164568c23bd7849c56450f32 


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


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 70846: SENTRY-2240: User can DROP function under a database that he/she has no access

[Arjun Mishra via Review Board]

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


Ship it!




Ship It!

- Arjun Mishra


On June 14, 2019, 2:58 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70846/
> ---
> 
> (Updated June 14, 2019, 2:58 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda and Na Li.
> 
> 
> Bugs: SENTRY-2240
> https://issues.apache.org/jira/browse/SENTRY-2240
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> User can DROP UDF function under a database that he/she has no access to.
> 
> I created it as separate JIRA from SENTRY-781 due to changes are quite 
> different.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  e87d0f664fd6cf93b3b86a61b57f148827e5692f 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  ed278c8d68c415198f40bed62cfa757fa5a9 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
>  1aaa9b3fcade6ebcefcea269b3bd919fb47a44f6 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegesAtFunctionScope.java
>  bd0f978e86733b37cf3343c9841304fd61f9dcab 
> 
> 
> Diff: https://reviews.apache.org/r/70846/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 70846: SENTRY-2240: User can DROP function under a database that he/she has no access

[Arjun Mishra via Review Board]

> On June 14, 2019, 4:36 p.m., Na Li wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
> > Line 230 (original), 226 (patched)
> > 
> >
> > change comment to be consistent

Will do


> On June 14, 2019, 4:36 p.m., Na Li wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegesAtFunctionScope.java
> > Line 120 (original), 123 (patched)
> > 
> >
> > can you add test case for creating function fail due to not having 
> > privilege?

I don't need to add test cases for create function since I am not changing 
create functionality


- Arjun


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


On June 14, 2019, 2:58 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70846/
> ---
> 
> (Updated June 14, 2019, 2:58 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda and Na Li.
> 
> 
> Bugs: SENTRY-2240
> https://issues.apache.org/jira/browse/SENTRY-2240
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> User can DROP UDF function under a database that he/she has no access to.
> 
> I created it as separate JIRA from SENTRY-781 due to changes are quite 
> different.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  e87d0f664fd6cf93b3b86a61b57f148827e5692f 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  ed278c8d68c415198f40bed62cfa757fa5a9 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
>  1aaa9b3fcade6ebcefcea269b3bd919fb47a44f6 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegesAtFunctionScope.java
>  bd0f978e86733b37cf3343c9841304fd61f9dcab 
> 
> 
> Diff: https://reviews.apache.org/r/70846/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 70846: SENTRY-2240: User can DROP function under a database that he/she has no access

[Arjun Mishra via Review Board]

> On June 14, 2019, 4:36 p.m., Na Li wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
> > Lines 228 (patched)
> > 
> >
> > do you want to change for create function as well to get true currDB?
> > 
> >   case HiveParser.TOK_CREATEFUNCTION:
> > String udfClassName = 
> > BaseSemanticAnalyzer.unescapeSQLString(ast.getChild(1).getText());
> > try {
> >   CodeSource udfSrc =
> >   Class.forName(udfClassName, true, 
> > Utilities.getSessionSpecifiedClassLoader())
> >   .getProtectionDomain().getCodeSource();
> >   if (udfSrc == null) {
> > throw new SemanticException("Could not resolve the jar for 
> > UDF class " + udfClassName);
> >   }
> >   String udfJar = udfSrc.getLocation().getPath();
> >   if (udfJar == null || udfJar.isEmpty()) {
> > throw new SemanticException("Could not find the jar for UDF 
> > class " + udfClassName +
> > "to validate privileges");
> >   }
> >   udfURIs.add(parseURI(udfSrc.getLocation().toString(), true));
> > } catch (ClassNotFoundException e) {
> >   List functionJars = getFunctionJars(ast);
> >   if (functionJars.isEmpty()) {
> > throw new SemanticException("Error retrieving udf class:" + 
> > e.getMessage(), e);
> >   } else {
> > // Add the jars from the command "Create function using 
> > jar" to the access list
> > // Defer to hive to check if the class is in the jars
> > for(String jar : functionJars) {
> >   udfURIs.add(parseURI(jar, false));
> > }
> >   }
> > }
> > 
> > // create/drop function is allowed with any database
> > currDB = Database.ALL;

I could but we are trying to limit the changes to very minimum. Adding changes 
to create function will increase the scope of this fix


- Arjun


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


On June 14, 2019, 2:58 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70846/
> ---
> 
> (Updated June 14, 2019, 2:58 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda and Na Li.
> 
> 
> Bugs: SENTRY-2240
> https://issues.apache.org/jira/browse/SENTRY-2240
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> User can DROP UDF function under a database that he/she has no access to.
> 
> I created it as separate JIRA from SENTRY-781 due to changes are quite 
> different.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
>  e87d0f664fd6cf93b3b86a61b57f148827e5692f 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  ed278c8d68c415198f40bed62cfa757fa5a9 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
>  1aaa9b3fcade6ebcefcea269b3bd919fb47a44f6 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegesAtFunctionScope.java
>  bd0f978e86733b37cf3343c9841304fd61f9dcab 
> 
> 
> Diff: https://reviews.apache.org/r/70846/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 70846: SENTRY-2240: User can DROP function under a database that he/she has no access

[Arjun Mishra via Review Board]

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

(Updated June 14, 2019, 2:58 p.m.)


Review request for sentry, kalyan kumar kalvagadda and Na Li.


Changes
---

Added more tests


Bugs: SENTRY-2240
https://issues.apache.org/jira/browse/SENTRY-2240


Repository: sentry


Description
---

User can DROP UDF function under a database that he/she has no access to.

I created it as separate JIRA from SENTRY-781 due to changes are quite 
different.


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
 e87d0f664fd6cf93b3b86a61b57f148827e5692f 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 ed278c8d68c415198f40bed62cfa757fa5a9 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
 1aaa9b3fcade6ebcefcea269b3bd919fb47a44f6 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegesAtFunctionScope.java
 bd0f978e86733b37cf3343c9841304fd61f9dcab 


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

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


Testing
---


Thanks,

Arjun Mishra

Review Request 70846: SENTRY-2240: User can DROP function under a database that he/she has no access

[Arjun Mishra via Review Board]

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

Review request for sentry, kalyan kumar kalvagadda and Na Li.


Repository: sentry


Description
---

User can DROP UDF function under a database that he/she has no access to.

I created it as separate JIRA from SENTRY-781 due to changes are quite 
different.


Diffs
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
 e87d0f664fd6cf93b3b86a61b57f148827e5692f 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegesAtFunctionScope.java
 bd0f978e86733b37cf3343c9841304fd61f9dcab 


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


Testing
---


Thanks,

Arjun Mishra

Review Request 70337: SENTRY-2511: Debug level logging on HMSPaths significantly affects performance

[Arjun Mishra via Review Board]

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

Review request for sentry and kalyan kumar kalvagadda.


Bugs: SENTRY-2511
https://issues.apache.org/jira/browse/SENTRY-2511


Repository: sentry


Description
---

Newer logging changes were made to HMSPath to help identify the corrupt cache. 
However when there are large number of partitions logging changes made makes 
the processing or creation of an update very slow


Diffs
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
 c49e24427 


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


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 70068: SENTRY-2503: Failed to revoke the privilege from impala-shell if the privilege added from beeline cli

[Arjun Mishra via Review Board]

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


Ship it!




Ship It!

- Arjun Mishra


On March 4, 2019, 9:49 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70068/
> ---
> 
> (Updated March 4, 2019, 9:49 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2503
> https://issues.apache.org/jira/browse/sentry-2503
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When granting all privilege on a table from impala-shell, this privilege 
> cannot be revoked from hiove beeline.
> 
> When granting all privilege on a table from hive beeline, this privilege 
> cannot be revoked from impala-shell.
> 
> The reason is that impala uses "ALL" to represent all privilege, and hive 
> uses "*". So getting privilege to drop it does not properly.
> 
> The solution is to find all equivalent privileges from input, and drop them.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  240120c 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1d97ff6 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  38b4c87 
> 
> 
> Diff: https://reviews.apache.org/r/70068/diff/4/
> 
> 
> Testing
> ---
> 
> add new tests and all old tests in TestSentryStore pass
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 70068: SENTRY-2503: Failed to revoke the privilege from impala-shell if the privilege added from beeline cli

[Arjun Mishra via Review Board]

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



Fix the annotation and ship it

- Arjun Mishra


On March 4, 2019, 8:46 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70068/
> ---
> 
> (Updated March 4, 2019, 8:46 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2503
> https://issues.apache.org/jira/browse/sentry-2503
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When granting all privilege on a table from impala-shell, this privilege 
> cannot be revoked from hiove beeline.
> 
> When granting all privilege on a table from hive beeline, this privilege 
> cannot be revoked from impala-shell.
> 
> The reason is that impala uses "ALL" to represent all privilege, and hive 
> uses "*". So getting privilege to drop it does not properly.
> 
> The solution is to find all equivalent privileges from input, and drop them.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  240120c 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1d97ff6 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  38b4c87 
> 
> 
> Diff: https://reviews.apache.org/r/70068/diff/3/
> 
> 
> Testing
> ---
> 
> add new tests and all old tests in TestSentryStore pass
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 70068: SENTRY-2503: Failed to revoke the privilege from impala-shell if the privilege added from beeline cli

[Arjun Mishra via Review Board]

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




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


You need to add @Test here


- Arjun Mishra


On March 4, 2019, 8:46 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70068/
> ---
> 
> (Updated March 4, 2019, 8:46 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2503
> https://issues.apache.org/jira/browse/sentry-2503
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When granting all privilege on a table from impala-shell, this privilege 
> cannot be revoked from hiove beeline.
> 
> When granting all privilege on a table from hive beeline, this privilege 
> cannot be revoked from impala-shell.
> 
> The reason is that impala uses "ALL" to represent all privilege, and hive 
> uses "*". So getting privilege to drop it does not properly.
> 
> The solution is to find all equivalent privileges from input, and drop them.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  240120c 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1d97ff6 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  38b4c87 
> 
> 
> Diff: https://reviews.apache.org/r/70068/diff/3/
> 
> 
> Testing
> ---
> 
> add new tests and all old tests in TestSentryStore pass
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 70076: SENTRY-2505: Fix file bounds in TestRollingFileWithoutDeleteAppender test case testFileNamePattern

[Arjun Mishra via Review Board]

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

(Updated March 4, 2019, 6:02 p.m.)


Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, and Na Li.


Changes
---

Post feedback


Bugs: SENTRY-2505
https://issues.apache.org/jira/browse/SENTRY-2505


Repository: sentry


Description
---

TestRollingFileWithoutDeleteAppender#testFileNamePattern still flaky because of 
size bounds


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/appender/TestRollingFileWithoutDeleteAppender.java
 6ee6b08a0d7d503a9dd5b7c8fde583b944194414 


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

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


Testing
---


Thanks,

Arjun Mishra

Review Request 70076: SENTRY-2505: Fix file bounds in TestRollingFileWithoutDeleteAppender test case testFileNamePattern

[Arjun Mishra via Review Board]

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

Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, and Na Li.


Bugs: SENTRY-2505
https://issues.apache.org/jira/browse/SENTRY-2505


Repository: sentry


Description
---

TestRollingFileWithoutDeleteAppender#testFileNamePattern still flaky because of 
size bounds


Diffs
-

  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/appender/TestRollingFileWithoutDeleteAppender.java
 6ee6b08a0 


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


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 70076: SENTRY-2505: Fix file bounds in TestRollingFileWithoutDeleteAppender test case testFileNamePattern

[Arjun Mishra via Review Board]

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

(Updated Feb. 28, 2019, 6:57 p.m.)


Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, and Na Li.


Bugs: SENTRY-2505
https://issues.apache.org/jira/browse/SENTRY-2505


Repository: sentry


Description
---

TestRollingFileWithoutDeleteAppender#testFileNamePattern still flaky because of 
size bounds


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/appender/TestRollingFileWithoutDeleteAppender.java
 6ee6b08a0 


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

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


Testing
---


Thanks,

Arjun Mishra

Review Request 70074: SENTRY-2504: Account for partial revokes on ALL grant

[Arjun Mishra via Review Board]

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

Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, and Na Li.


Bugs: SENTRY-2504
https://issues.apache.org/jira/browse/SENTRY-2504


Repository: sentry


Description
---

Right now if ALL grant is given to a role+object, if we revoke a SELECT or 
INSERT, we don't replace with a partial privilege. This however works with *


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 1d97ff6237846c8e2a0a70059191f2987e814667 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 38b4c879bf953527b6e9341dee6dd80d38e6349f 


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


Testing
---

$ mvn -f sentry-service/sentry-service-server/pom.xml test 
-Dtest=TestSentryStore


Thanks,

Arjun Mishra

Re: Review Request 70068: SENTRY-2503: Failed to revoke the privilege from impala-shell if the privilege added from beeline cli

[Arjun Mishra via Review Board]

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


Ship it!




Will fix the issue with partial revokes on ALL on a separate ticket

- Arjun Mishra


On Feb. 27, 2019, 7:28 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70068/
> ---
> 
> (Updated Feb. 27, 2019, 7:28 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2503
> https://issues.apache.org/jira/browse/sentry-2503
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When granting all privilege on a table from impala-shell, this privilege 
> cannot be revoked from hiove beeline.
> 
> When granting all privilege on a table from hive beeline, this privilege 
> cannot be revoked from impala-shell.
> 
> The reason is that impala uses "ALL" to represent all privilege, and hive 
> uses "*". So getting privilege to drop it does not properly.
> 
> The solution is to find all equivalent privileges from input, and drop them.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  240120c 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1d97ff6 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  38b4c87 
> 
> 
> Diff: https://reviews.apache.org/r/70068/diff/1/
> 
> 
> Testing
> ---
> 
> add new tests and all old tests in TestSentryStore pass
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 70068: SENTRY-2503: Failed to revoke the privilege from impala-shell if the privilege added from beeline cli

[Arjun Mishra via Review Board]

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



Lina, the fix only covers ALL/* revoke. But if you had a ALL on Impala and you 
revoked SELET on Hive it wouldn't work. That case is still missing from this 
fix.

- Arjun Mishra


On Feb. 27, 2019, 7:28 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70068/
> ---
> 
> (Updated Feb. 27, 2019, 7:28 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2503
> https://issues.apache.org/jira/browse/sentry-2503
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When granting all privilege on a table from impala-shell, this privilege 
> cannot be revoked from hiove beeline.
> 
> When granting all privilege on a table from hive beeline, this privilege 
> cannot be revoked from impala-shell.
> 
> The reason is that impala uses "ALL" to represent all privilege, and hive 
> uses "*". So getting privilege to drop it does not properly.
> 
> The solution is to find all equivalent privileges from input, and drop them.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  240120c 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1d97ff6 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  38b4c87 
> 
> 
> Diff: https://reviews.apache.org/r/70068/diff/1/
> 
> 
> Testing
> ---
> 
> add new tests and all old tests in TestSentryStore pass
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69987: SENTRY-2497: show grant role results should handle case where URI doesn't have a defined scheme

[Arjun Mishra via Review Board]

> On Feb. 15, 2019, 6:52 p.m., Arjun Mishra wrote:
> > Haley I think we can simplify this a lot without any refactoring by using 
> > PathUtils#parseLocalURI method.
> > 
> > Since the method is isLocalUri => It is looking to check for a local URI. 
> > If uri.getScheme() throws null we can set uri = 
> > PathUtils.parseLocalURI(path). If that is still null we return false. 
> > 
> > Let me know your thoughts
> 
> Haley Reeve wrote:
> That's an alternate way of doing it. In that case the only way we'd get 
> to false is to catch the IllegalArgumentException PathUtils#parseLocalURI 
> throws. I don't think it greatly simplifies anything except that we don't 
> have to pass down the HiveAuthzConf anymore. Is adding that parameter to the 
> function definitions undesirable?

Yeah I think so. It is too much of refactoring for not much gain. We want to 
check if it is a local URI. If You can't parse a local URI then it definitely 
isnt. So that should be sufficient. 
If you want you can directly instantiate: URI uri = new 
URI(PathUtils.parseLocalURI(uriString)); and catch exception.
Will simplify code


- Arjun


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


On Feb. 14, 2019, 4:36 p.m., Haley Reeve wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69987/
> ---
> 
> (Updated Feb. 14, 2019, 4:36 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The "show grant role" logic tries to use a URI's scheme to tell whether it's 
> a local URI or a DFS URI. However, it's valid for the scheme to be undefined. 
> In that case Sentry throws a NPE because it's trying to access a null scheme.
> 
> The logic has been updated to instead use the default filesystem set in the 
> "fs.defaultFS" property if the scheme is not defined.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
>  94783fa 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
>  5996b6c 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java
>  6a7c1f3 
> 
> 
> Diff: https://reviews.apache.org/r/69987/diff/1/
> 
> 
> Testing
> ---
> 
> Created and ran testShowGrantWithNullScheme() unit test. Checked that test 
> fails without code change, and succeeds with code change.
> 
> 
> Thanks,
> 
> Haley Reeve
> 
>

Re: Review Request 69987: SENTRY-2497: show grant role results should handle case where URI doesn't have a defined scheme

[Arjun Mishra via Review Board]

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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
Line 283 (original), 285 (patched)


Use PathUtils#parseLocalURI method if uri.getScheme is null to 
re-intantiate the object. That should work


- Arjun Mishra


On Feb. 14, 2019, 4:36 p.m., Haley Reeve wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69987/
> ---
> 
> (Updated Feb. 14, 2019, 4:36 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The "show grant role" logic tries to use a URI's scheme to tell whether it's 
> a local URI or a DFS URI. However, it's valid for the scheme to be undefined. 
> In that case Sentry throws a NPE because it's trying to access a null scheme.
> 
> The logic has been updated to instead use the default filesystem set in the 
> "fs.defaultFS" property if the scheme is not defined.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
>  94783fa 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
>  5996b6c 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java
>  6a7c1f3 
> 
> 
> Diff: https://reviews.apache.org/r/69987/diff/1/
> 
> 
> Testing
> ---
> 
> Created and ran testShowGrantWithNullScheme() unit test. Checked that test 
> fails without code change, and succeeds with code change.
> 
> 
> Thanks,
> 
> Haley Reeve
> 
>

Re: Review Request 69987: SENTRY-2497: show grant role results should handle case where URI doesn't have a defined scheme

[Arjun Mishra via Review Board]

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



Haley I think we can simplify this a lot without any refactoring by using 
PathUtils#parseLocalURI method.

Since the method is isLocalUri => It is looking to check for a local URI. If 
uri.getScheme() throws null we can set uri = PathUtils.parseLocalURI(path). If 
that is still null we return false. 

Let me know your thoughts

- Arjun Mishra


On Feb. 14, 2019, 4:36 p.m., Haley Reeve wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69987/
> ---
> 
> (Updated Feb. 14, 2019, 4:36 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The "show grant role" logic tries to use a URI's scheme to tell whether it's 
> a local URI or a DFS URI. However, it's valid for the scheme to be undefined. 
> In that case Sentry throws a NPE because it's trying to access a null scheme.
> 
> The logic has been updated to instead use the default filesystem set in the 
> "fs.defaultFS" property if the scheme is not defined.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
>  94783fa 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
>  5996b6c 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java
>  6a7c1f3 
> 
> 
> Diff: https://reviews.apache.org/r/69987/diff/1/
> 
> 
> Testing
> ---
> 
> Created and ran testShowGrantWithNullScheme() unit test. Checked that test 
> fails without code change, and succeeds with code change.
> 
> 
> Thanks,
> 
> Haley Reeve
> 
>

Review Request 69941: SENTRY-2494: Fix TestRollingFileWithoutDeleteAppender test case testFileNamePattern

[Arjun Mishra via Review Board]

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

Review request for sentry and kalyan kumar kalvagadda.


Bugs: SENTRY-2494
https://issues.apache.org/jira/browse/SENTRY-2494


Repository: sentry


Description
---

The log size is set to 10 bytes. However if the message size is 15 bytes, it 
creates a 15, 15 and 0 byte file ( which is sometimes flaky)

Explanation:
Before we logged a string that was at 15 bytes each. The assumption was Logger 
would split that across 2 files but it never did that. It would put 15 bytes of 
line on one file.

Previously we had 2 log statements:
debug."123456789012345"; 
debug."123456789012345";

The file being created was "123456789012345", "123456789012345", "" (LAST ONE 
empty)

as opposed to "1234567890", "1234512345", "6789012345"

The above output would be flaky because LOGGER.appender did not handle a LONG 
string properly. It would sometimes generate two files with "123456789012345", 
"" (LAST ONE empty)


Diffs
-

  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/appender/TestRollingFileWithoutDeleteAppender.java
 5db15a3d0e78a071ec10c9fba4048c95f2f0cc89 


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


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 65866: SENTRY-2146: Add better error handling to ResourceAuthorizationProvider and improve logging in related classes

[Arjun Mishra via Review Board]

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

(Updated Feb. 6, 2019, 7:35 p.m.)


Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, Steve 
Moist, and Sergio Pena.


Bugs: SENTRY-2146
https://issues.apache.org/jira/browse/SENTRY-2146


Repository: sentry


Description
---

There are a bunch of improvements that should be made to 
ResourceAuthorizationProvider. For example, exceptions thrown by 
privilegeFactory.createPrivilege are not gracefully handled. Makes debugging 
hard. 

We also need to add a lot more logging that needs to be added to related classes


Diffs
-

  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
 7565a34b5 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
 09bd9b566 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 447deaf58 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
 4e944e5f2 
  
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
 ab5560994 
  
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
 a9b98f319 


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


Testing
---

mvn -f sentry-binding/sentry-binding-hive/pom.xml test
mvn -f sentry-core/sentry-core-common/pom.xml test
mvn -f sentry-policy/sentry-policy-common/pom.xml test
mvn -f sentry-policy/sentry-policy-engine/pom.xml test
mvn -f sentry-provider/sentry-provider-common/pom.xml test


File Attachments (updated)


New diff. Not able to update
  
https://reviews.apache.org/media/uploaded/files/2019/02/06/5b613338-7181-4bbe-bac5-66129cdbc095__SENTRY-2146.04.patch


Thanks,

Arjun Mishra

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[Arjun Mishra via Review Board]

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




sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 519 (original), 542 (patched)


clearAuthzObjs() is setting authzObjs to null. Can we add that to the log 
message: 
LOG.debug("Entry with path: {} is changed to DIR. Setting authzObjs to 
null", this.getFullPath());



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Lines 211 (patched)


This is not part of the logging. Please add. Right now you are just 
returning the sequenceInfo but not logging it


- Arjun Mishra


On Feb. 5, 2019, 12:01 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Feb. 5, 2019, 12:01 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as part of the cache.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryAuthzUpdate.java
>  db11c1e1ad7d11d936bcb5f5d29ad105471394d7 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  c967f4e2ee0a811e04362aef452b1cfddfd973f3 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9dae03562aff82574dcda5339df415193655d5a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  5726c0e834e6e5ad47f47f3337d9317ee1366955 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  9115697088cab8fd1732086963c7c3f982069380 
> 
> 
> Diff: https://reviews.apache.org/r/69875/diff/3/
> 
> 
> Testing
> ---
> 
> As there is no functiuonal change I have not added any tests. I made sure 
> that existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[Arjun Mishra via Review Board]

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


Ship it!




- Arjun Mishra


On Feb. 5, 2019, 12:01 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Feb. 5, 2019, 12:01 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as part of the cache.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryAuthzUpdate.java
>  db11c1e1ad7d11d936bcb5f5d29ad105471394d7 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  c967f4e2ee0a811e04362aef452b1cfddfd973f3 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9dae03562aff82574dcda5339df415193655d5a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  5726c0e834e6e5ad47f47f3337d9317ee1366955 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  9115697088cab8fd1732086963c7c3f982069380 
> 
> 
> Diff: https://reviews.apache.org/r/69875/diff/3/
> 
> 
> Testing
> ---
> 
> As there is no functiuonal change I have not added any tests. I made sure 
> that existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[Arjun Mishra via Review Board]

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


Ship it!




Left last bits of comments. Please fix them and shit it

- Arjun Mishra


On Feb. 5, 2019, 12:01 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Feb. 5, 2019, 12:01 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as part of the cache.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryAuthzUpdate.java
>  db11c1e1ad7d11d936bcb5f5d29ad105471394d7 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  c967f4e2ee0a811e04362aef452b1cfddfd973f3 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9dae03562aff82574dcda5339df415193655d5a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  5726c0e834e6e5ad47f47f3337d9317ee1366955 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  9115697088cab8fd1732086963c7c3f982069380 
> 
> 
> Diff: https://reviews.apache.org/r/69875/diff/3/
> 
> 
> Testing
> ---
> 
> As there is no functiuonal change I have not added any tests. I made sure 
> that existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[Arjun Mishra via Review Board]

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




sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 123 (original), 125 (patched)


Why do you need both isDebugEnabled and isTraceEnabled?


- Arjun Mishra


On Feb. 5, 2019, 12:01 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Feb. 5, 2019, 12:01 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as part of the cache.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryAuthzUpdate.java
>  db11c1e1ad7d11d936bcb5f5d29ad105471394d7 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  c967f4e2ee0a811e04362aef452b1cfddfd973f3 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9dae03562aff82574dcda5339df415193655d5a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  5726c0e834e6e5ad47f47f3337d9317ee1366955 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  9115697088cab8fd1732086963c7c3f982069380 
> 
> 
> Diff: https://reviews.apache.org/r/69875/diff/3/
> 
> 
> Testing
> ---
> 
> As there is no functiuonal change I have not added any tests. I made sure 
> that existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[Arjun Mishra via Review Board]

> On Feb. 5, 2019, 8:25 p.m., kalyan kumar kalvagadda wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
> > Line 124 (original), 124 (patched)
> > 
> >
> > why? What is missing new the one?
> > Looking at the previous one can not know what numbers mean. With new 
> > one, you can see what is the perm sequence number ,path sequence number and 
> > path image number.

There are 3 {} but 4 arguments. You need one more {} for perm updates size. 
Also nit pick: Can you change NUmber to Number


- Arjun


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


On Feb. 5, 2019, 12:01 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Feb. 5, 2019, 12:01 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as part of the cache.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryAuthzUpdate.java
>  db11c1e1ad7d11d936bcb5f5d29ad105471394d7 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  c967f4e2ee0a811e04362aef452b1cfddfd973f3 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9dae03562aff82574dcda5339df415193655d5a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  5726c0e834e6e5ad47f47f3337d9317ee1366955 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  9115697088cab8fd1732086963c7c3f982069380 
> 
> 
> Diff: https://reviews.apache.org/r/69875/diff/3/
> 
> 
> Testing
> ---
> 
> As there is no functiuonal change I have not added any tests. I made sure 
> that existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[Arjun Mishra via Review Board]

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




sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 125 (original), 125 (patched)


I prefer the old way of logging


- Arjun Mishra


On Feb. 1, 2019, 10:22 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Feb. 1, 2019, 10:22 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as part of the cache.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  c967f4e2ee0a811e04362aef452b1cfddfd973f3 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9dae03562aff82574dcda5339df415193655d5a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  5726c0e834e6e5ad47f47f3337d9317ee1366955 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  9115697088cab8fd1732086963c7c3f982069380 
> 
> 
> Diff: https://reviews.apache.org/r/69875/diff/2/
> 
> 
> Testing
> ---
> 
> As there is no functiuonal change I have not added any tests. I made sure 
> that existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[Arjun Mishra via Review Board]

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




sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
Line 117 (original), 117 (patched)


Log message here to say renaming, just like below where we did for adding 
and removing



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 149 (original)


We need this lock right?



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 199 (original), 203 (patched)


Better to keep old information. As it is more verbose



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 209 (original), 213 (patched)


Same as above



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 216 (original), 218 (patched)


Same as above



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 236 (original), 235 (patched)


I thought we are changing this to Throwable???


- Arjun Mishra


On Feb. 1, 2019, 10:22 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Feb. 1, 2019, 10:22 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as part of the cache.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  c967f4e2ee0a811e04362aef452b1cfddfd973f3 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9dae03562aff82574dcda5339df415193655d5a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  5726c0e834e6e5ad47f47f3337d9317ee1366955 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  9115697088cab8fd1732086963c7c3f982069380 
> 
> 
> Diff: https://reviews.apache.org/r/69875/diff/2/
> 
> 
> Testing
> ---
> 
> As there is no functiuonal change I have not added any tests. I made sure 
> that existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[Arjun Mishra via Review Board]

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




sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Lines 202 (patched)


Can we add method name?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Lines 224 (patched)


Can we add method name?



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 125 (original), 125 (patched)


This is not right. More paranthesis than arguments


- Arjun Mishra


On Feb. 1, 2019, 10:22 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Feb. 1, 2019, 10:22 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as part of the cache.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  c967f4e2ee0a811e04362aef452b1cfddfd973f3 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9dae03562aff82574dcda5339df415193655d5a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  5726c0e834e6e5ad47f47f3337d9317ee1366955 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  9115697088cab8fd1732086963c7c3f982069380 
> 
> 
> Diff: https://reviews.apache.org/r/69875/diff/2/
> 
> 
> Testing
> ---
> 
> As there is no functiuonal change I have not added any tests. I made sure 
> that existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[Arjun Mishra via Review Board]

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




sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
Lines 118 (patched)


This is not required as renameAuthzObject has the same log message



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 199 (original), 203 (patched)


Why not keep seq num and image num also class name?



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 209 (original), 213 (patched)


Same as above



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 216 (original), 218 (patched)


Same as above



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 124 (original), 124 (patched)


I prefer the old way of logging


- Arjun Mishra


On Jan. 31, 2019, 4:46 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Jan. 31, 2019, 4:46 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as part of the cache.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  c967f4e2ee0a811e04362aef452b1cfddfd973f3 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9dae03562aff82574dcda5339df415193655d5a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  5726c0e834e6e5ad47f47f3337d9317ee1366955 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  9115697088cab8fd1732086963c7c3f982069380 
> 
> 
> Diff: https://reviews.apache.org/r/69875/diff/1/
> 
> 
> Testing
> ---
> 
> As there is no functiuonal change I have not added any tests. I made sure 
> that existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[Arjun Mishra via Review Board]

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



We definitely need logs on Entry class

- Arjun Mishra


On Jan. 31, 2019, 4:46 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Jan. 31, 2019, 4:46 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as part of the cache.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  c967f4e2ee0a811e04362aef452b1cfddfd973f3 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9dae03562aff82574dcda5339df415193655d5a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  5726c0e834e6e5ad47f47f3337d9317ee1366955 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  9115697088cab8fd1732086963c7c3f982069380 
> 
> 
> Diff: https://reviews.apache.org/r/69875/diff/1/
> 
> 
> Testing
> ---
> 
> As there is no functiuonal change I have not added any tests. I made sure 
> that existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[Arjun Mishra via Review Board]

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




sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Lines 748 (patched)


Adding method name will be helpful here



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Lines 751 (patched)


Adding method name will be helpful here



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 771 (original)


I prefer the old way of logging. Easy to track



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 800 (original)


Prefer the old way. You can add the size information to existing log message


- Arjun Mishra


On Jan. 31, 2019, 4:46 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Jan. 31, 2019, 4:46 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as part of the cache.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  c967f4e2ee0a811e04362aef452b1cfddfd973f3 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9dae03562aff82574dcda5339df415193655d5a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  5726c0e834e6e5ad47f47f3337d9317ee1366955 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  9115697088cab8fd1732086963c7c3f982069380 
> 
> 
> Diff: https://reviews.apache.org/r/69875/diff/1/
> 
> 
> Testing
> ---
> 
> As there is no functiuonal change I have not added any tests. I made sure 
> that existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[Arjun Mishra via Review Board]

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




sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 206 (original), 207 (patched)


Can you add a log message here?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 222 (original), 223 (patched)


We need a DEBUG level log here



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 236 (original), 237 (patched)


We need a DEBUG level log here



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 256 (original), 257 (patched)


Same here



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 358 (original), 359 (patched)


Can you add log here?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 387 (original), 388 (patched)


Can you add log here?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 412 (original), 413 (patched)


Log here?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 425 (original), 426 (patched)


Log here?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 435 (original), 436 (patched)


Same here?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Lines 454 (patched)


Can we change the convention to method name then other information? It'll 
be easy to track. Thoughts?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Lines 464 (patched)


Same as above method name + arguments - followed by message



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 468 (original), 472 (patched)


Log here?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Lines 476 (patched)


This should be logged before the method



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 481 (original), 487 (patched)


Log here?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Lines 502 (patched)


Same here?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 511 (original), 518 (patched)


Log here?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 526 (original), 535 (patched)


Log here?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 695 (original), 704 (patched)


I prefer the old way of logging. Logs the messag that is called. Its easier 
to track



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 725 (original)


I prefer the old way of logging


- Arjun Mishra


On Jan. 31, 2019, 4:46 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Jan. 31, 2019, 4:46 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as

Re: Review Request 69875: SENTRY-2205: Improve Sentry NN Logging

[Arjun Mishra via Review Board]

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



General comment - Instead of logging sentances can we simplify it like method 
name and attributes? Also add that to the start of every method? What do you 
think?
For example: "putChild({},{})", or "deleteFromParent: Removing path {}"

- Arjun Mishra


On Jan. 31, 2019, 4:46 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69875/
> ---
> 
> (Updated Jan. 31, 2019, 4:46 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and Na Li.
> 
> 
> Bugs: SENTRY-2205
> https://issues.apache.org/jira/browse/SENTRY-2205
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Logging will help us identify below
> 1. Track the updates from Sentry
> 2. What the path update is?
> 3. What the perm update is?
> 4. How the path change is effecting the exizting cache and what is the result 
> of the update?
> 5. How the perm change is effecting the exizting cache and what is the result 
> of the update?
> 6. Changes to the path tree stored as part of the cache.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  97a04d9eb5a1e0c5feb97b038b8fc0edec9b0d38 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  bbf3f13e44ca3ee6ecb1d635abb9048942b26ac3 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  c967f4e2ee0a811e04362aef452b1cfddfd973f3 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  9dae03562aff82574dcda5339df415193655d5a2 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
>  5726c0e834e6e5ad47f47f3337d9317ee1366955 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  9115697088cab8fd1732086963c7c3f982069380 
> 
> 
> Diff: https://reviews.apache.org/r/69875/diff/1/
> 
> 
> Testing
> ---
> 
> As there is no functiuonal change I have not added any tests. I made sure 
> that existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

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

[Arjun Mishra via Review Board]

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

(Updated Jan. 29, 2019, 9:09 p.m.)


Review request for sentry, Haley Reeve, 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 (updated)
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
 2d21411e2 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
 534bb51c1 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
 4ff3dc91a 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 4baeb6725 


Diff: https://reviews.apache.org/r/69076/diff/5/

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


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 69832: SENTRY-2488: Add privilege cache to sentry hive bindings in DefaultAccessValidator

[Arjun Mishra via Review Board]

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

(Updated Jan. 29, 2019, 6:47 p.m.)


Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, and Na Li.


Changes
---

Fixed a bug where we try to clear an Immutable Set.


Bugs: SENTRY-2488
https://issues.apache.org/jira/browse/SENTRY-2488


Repository: sentry


Description
---

We are not consistent with behavior in SentryHiveMetaStoreHook (not used 
anymore) which would cache privileges when authorizing show databases or show 
tables command. This needs to be added back


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
 9de47b338 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 63d8d1c09 
  
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java
 0ad4616c0 


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

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


Testing
---


Thanks,

Arjun Mishra

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

[Arjun Mishra via Review Board]

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

(Updated Jan. 29, 2019, 3:43 p.m.)


Review request for sentry, Haley Reeve, 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 (updated)
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
 2d21411e2 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
 534bb51c1 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
 4ff3dc91a 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 4baeb6725 


Diff: https://reviews.apache.org/r/69076/diff/4/

Changes: https://reviews.apache.org/r/69076/diff/3-4/


Testing
---


Thanks,

Arjun Mishra

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

[Arjun Mishra via Review Board]

> On Jan. 24, 2019, 11:52 p.m., kalyan kumar kalvagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
> > Lines 107 (patched)
> > 
> >
> > Can you make configurarble? It might be helpfull.
> >  What do you say?

Sure


- Arjun


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


On Jan. 24, 2019, 6:24 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69076/
> ---
> 
> (Updated Jan. 24, 2019, 6:24 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, 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
>  534bb51c1 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  4ff3dc91a 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  4baeb6725 
> 
> 
> Diff: https://reviews.apache.org/r/69076/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69841: SENTRY-2486: Wrong user name when sentry HMSFollower gets full snapshot from HMS at insecure mode

[Arjun Mishra via Review Board]

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


Ship it!




Ship It!

- Arjun Mishra


On Jan. 28, 2019, 6:55 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69841/
> ---
> 
> (Updated Jan. 28, 2019, 6:55 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2486
> https://issues.apache.org/jira/browse/sentry-2486
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In insecure mode, the current login user name is passed from Sentry to HMS 
> server when sentry HMSFollower gets full snapshot from HMS. 
> 
> The user name should be "sentry" instead of current login user.
> 
> This issue should not happen in production because secure mode is always 
> used. Insecure mode is only used in test.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
>  31e58fd 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  0d62941 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  47f7466 
> 
> 
> Diff: https://reviews.apache.org/r/69841/diff/2/
> 
> 
> Testing
> ---
> 
> Tested manually and verified the user name now is "sentry" when sentry 
> HMSFollower gets notifications from HMS server
> 
> 
> Thanks,
> 
> Na Li
> 
>

Review Request 69850: SENTRY-2490: When building a full perm update for each object we only build 1 privilege per role

[Arjun Mishra via Review Board]

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

Review request for sentry and kalyan kumar kalvagadda.


Bugs: SENTRY-2490
https://issues.apache.org/jira/browse/SENTRY-2490


Repository: sentry


Description
---

When building a perm full update we only include one privilege a role has on an 
object as opposed to the entire privilege set


Diffs
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
 9de47b338 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 63d8d1c09 


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


Testing
---

mvn -f sentry-service/sentry-service-server/pom.xml test -Dtest=TestSentryStore


Thanks,

Arjun Mishra

Re: Review Request 69840: SENTRY-2491: Sentry High availability unit tests run into deadlock sometimes

[Arjun Mishra via Review Board]

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


Ship it!




Ship It!

- Arjun Mishra


On Jan. 25, 2019, 7:57 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69840/
> ---
> 
> (Updated Jan. 25, 2019, 7:57 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, HaleyHH HaleyHH, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2491
> https://issues.apache.org/jira/browse/sentry-2491
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In sentry unit tests, we don't create schema before running a test. Instead, 
> we use dataNucleus to create sentry tables when they are accessed. This 
> creates potential deadlock when running test for Sentry HA setup.
> 
> The solution is to let the instances of sentry service start with delay. 
> Specifically,
> let HMS follower threads separate as far as possible, i.e., half of the 
> interval.
> 
> This deadlock only exists in unit tests, and does not exist in production 
> because schema is created before starting Sentry services. Therefore, there 
> is no table creation after service starts.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  9fa42f2 
> 
> 
> Diff: https://reviews.apache.org/r/69840/diff/1/
> 
> 
> Testing
> ---
> 
> such deadlock does not happen with this fix.
> Other unit tests passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69832: SENTRY-2488: Add privilege cache to sentry hive bindings in DefaultAccessValidator

[Arjun Mishra via Review Board]

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

(Updated Jan. 24, 2019, 9:35 p.m.)


Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, and Na Li.


Changes
---

Post feebdack


Bugs: SENTRY-2488
https://issues.apache.org/jira/browse/SENTRY-2488


Repository: sentry


Description
---

We are not consistent with behavior in SentryHiveMetaStoreHook (not used 
anymore) which would cache privileges when authorizing show databases or show 
tables command. This needs to be added back


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
 9de47b338 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 63d8d1c09 


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

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


Testing
---


Thanks,

Arjun Mishra

Review Request 69832: SENTRY-2488: Add privilege cache to sentry hive bindings in DefaultAccessValidator

[Arjun Mishra via Review Board]

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

Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, and Na Li.


Bugs: SENTRY-2488
https://issues.apache.org/jira/browse/SENTRY-2488


Repository: sentry


Description
---

We are not consistent with behavior in SentryHiveMetaStoreHook (not used 
anymore) which would cache privileges when authorizing show databases or show 
tables command. This needs to be added back


Diffs
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
 9de47b338 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 63d8d1c09 


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


Testing
---


Thanks,

Arjun Mishra

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

[Arjun Mishra via Review Board]

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

(Updated Jan. 18, 2019, 6:18 p.m.)


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


Changes
---

New patch


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

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
 534bb51c1 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
 4ff3dc91a 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 4baeb6725 


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

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


Testing
---


Thanks,

Arjun Mishra

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

[Arjun Mishra via Review Board]

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



- Arjun Mishra


On Jan. 11, 2019, 4:55 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69702/
> ---
> 
> (Updated Jan. 11, 2019, 4:55 a.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
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  328d2b5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
>  31e58fd 
>   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/3/
> 
> 
> Testing
> ---
> 
> add new e2e tests for READ_DATABASE and READ_TABLE at HMS
> 
> 
> Thanks,
> 
> Na Li
> 
>

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

[Arjun Mishra via Review Board]

> On Jan. 11, 2019, 4:59 a.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
> > Line 112 (original), 117 (patched)
> > 
> >
> > If I don't make this change, in insecured mode, the user will be 
> > current login user. It will be hard to configure super user at HMS server 
> > to bypass authoriozation check at readiong metadata.
> > 
> > The value of realm is not important as only short user name is checked.

Why would it be the current login user? If it is insecure we wou;dn't have 
initialized the KerberonContext. Do we want to allow insecure connection 
between Sentry and HMS? I don't think we should be forcing this change. 
Instead of this you could remove the if (insecure) code block from the init 
method. That way the connection is always secure


- Arjun


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


On Jan. 11, 2019, 4:55 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69702/
> ---
> 
> (Updated Jan. 11, 2019, 4:55 a.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
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  328d2b5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
>  31e58fd 
>   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/3/
> 
> 
> Testing
> ---
> 
> add new e2e tests for READ_DATABASE and READ_TABLE at HMS
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69596: SENTRY-1797: SentryKerberosContext should use periodic executor instead of managing periodic execution via run() method.

[Arjun Mishra via Review Board]

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


Ship it!




Ship It!

- Arjun Mishra


On Jan. 4, 2019, 4:33 p.m., Haley Reeve wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69596/
> ---
> 
> (Updated Jan. 4, 2019, 4:33 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-1797
> https://issues.apache.org/jira/browse/SENTRY-1797
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The SentryKerberosContext class tries to simulate periodic executor using 
> while loop with a sleep in the run() method. Instead it should use periodic 
> executor.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java
>  efb8ae6 
> 
> 
> Diff: https://reviews.apache.org/r/69596/diff/2/
> 
> 
> Testing
> ---
> 
> Ran unit tests.
> 
> 
> Thanks,
> 
> Haley Reeve
> 
>

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

[Arjun Mishra via Review Board]

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


Ship it!




Fix failed tests

- Arjun Mishra


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

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

[Arjun Mishra via Review Board]

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



Looks good. Please fix the test cases. Would like to review the next patch as 
well.

- Arjun Mishra


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

Re: Review Request 69596: SENTRY-1797: SentryKerberosContext should use periodic executor instead of managing periodic execution via run() method.

[Arjun Mishra via Review Board]

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




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


Can you explain the reasoning behind 30L? Before we would check ever sec



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java
Line 47 (original), 46 (patched)


Going by SentryService class, could we use the ScheduledExecutorService 
class since it doesn't have to be boxed? Thoughts?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java
Line 148 (original), 132 (patched)


Can we could also keep the renewerThreadFactor, since it has an 
identifiable thread name "kerberos-renewer-%d". Then we could pass in the 
threadfactory into Executors.newScheduledThreadPool(1,renewerThreadFactory) 
method. Thoughts?


- Arjun Mishra


On Dec. 19, 2018, 5:25 p.m., Haley Reeve wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69596/
> ---
> 
> (Updated Dec. 19, 2018, 5:25 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-1797
> https://issues.apache.org/jira/browse/SENTRY-1797
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The SentryKerberosContext class tries to simulate periodic executor using 
> while loop with a sleep in the run() method. Instead it should use periodic 
> executor.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java
>  efb8ae6 
> 
> 
> Diff: https://reviews.apache.org/r/69596/diff/1/
> 
> 
> Testing
> ---
> 
> Ran unit tests.
> 
> 
> Thanks,
> 
> Haley Reeve
> 
>

Re: Review Request 69618: SENTRY-2469 Fix bugs in RoleServlet

[Arjun Mishra via Review Board]

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


Ship it!




Ship It!

- Arjun Mishra


On Dec. 21, 2018, 10:16 a.m., hongtaq zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69618/
> ---
> 
> (Updated Dec. 21, 2018, 10:16 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Anthony Young-Garner, kalyan kumar 
> kalvagadda, Li Pi, Na Li, Steve Moist, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2469
> https://issues.apache.org/jira/browse/SENTRY-2469
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> I have fixed issues in RoleServlet.java. 
> 
> 
> RoleServlet at L62 needs to use either try resource or put it in the finally 
> block to avoid resource leak
> if (conf.getBoolean(ServerConfig.SENTRY_WEB_ENABLE, 
> ServerConfig.SENTRY_WEB_ENABLE_DEFAULT)) may return an NPE if conf.getBoolean 
> returns a nullBoolean (not the primitive type)
> RoleServlet at L45, use Preconditions.checkNotNull or Objects.requreNonNull 
> instead of assert
> RoleServlet at L55: String json = new Gson().toJson(roleMap);, it's better to 
> reuse the Gson instance.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/RolesServlet.java
>  9ed2bf31 
> 
> 
> Diff: https://reviews.apache.org/r/69618/diff/2/
> 
> 
> Testing
> ---
> 
> builded and tested it.
> 
> 
> Thanks,
> 
> hongtaq zhao
> 
>

Re: Review Request 69618: SENTRY-2469 Fix bugs in RoleServlet

[Arjun Mishra via Review Board]

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




sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
Line 182 (original), 182 (patched)


This does't do anything. Its a final and its value is false. Why will it be 
NULL?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/RolesServlet.java
Line 55 (original), 55 (patched)


We are only using a single instance of Gson, why not leave as is?



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


This should really be the only change. Thoughts?


- Arjun Mishra


On Dec. 21, 2018, 10:16 a.m., hongtaq zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69618/
> ---
> 
> (Updated Dec. 21, 2018, 10:16 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Anthony Young-Garner, kalyan kumar 
> kalvagadda, Li Pi, Na Li, Steve Moist, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2469
> https://issues.apache.org/jira/browse/SENTRY-2469
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> I have fixed issues in RoleServlet.java. 
> 
> 
> RoleServlet at L62 needs to use either try resource or put it in the finally 
> block to avoid resource leak
> if (conf.getBoolean(ServerConfig.SENTRY_WEB_ENABLE, 
> ServerConfig.SENTRY_WEB_ENABLE_DEFAULT)) may return an NPE if conf.getBoolean 
> returns a nullBoolean (not the primitive type)
> RoleServlet at L45, use Preconditions.checkNotNull or Objects.requreNonNull 
> instead of assert
> RoleServlet at L55: String json = new Gson().toJson(roleMap);, it's better to 
> reuse the Gson instance.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  4508361b 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/RolesServlet.java
>  9ed2bf31 
> 
> 
> Diff: https://reviews.apache.org/r/69618/diff/1/
> 
> 
> Testing
> ---
> 
> builded and tested it.
> 
> 
> Thanks,
> 
> hongtaq zhao
> 
>

Re: Review Request 69618: SENTRY-2469 Fix bugs in RoleServlet

[Arjun Mishra via Review Board]

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



conf.getBoolean never returns NULL. See below
==
public boolean getBoolean(String name, boolean defaultValue) {
String valueString = this.getTrimmed(name);
if (null != valueString && !valueString.isEmpty()) {
  valueString = valueString.toLowerCase();
  if ("true".equals(valueString)) {
return true;
  } else {
return "false".equals(valueString) ? false : defaultValue;
  }
} else {
  return defaultValue;
}
  }
==

- Arjun Mishra


On Dec. 21, 2018, 10:16 a.m., hongtaq zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69618/
> ---
> 
> (Updated Dec. 21, 2018, 10:16 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Anthony Young-Garner, kalyan kumar 
> kalvagadda, Li Pi, Na Li, Steve Moist, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2469
> https://issues.apache.org/jira/browse/SENTRY-2469
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> I have fixed issues in RoleServlet.java. 
> 
> 
> RoleServlet at L62 needs to use either try resource or put it in the finally 
> block to avoid resource leak
> if (conf.getBoolean(ServerConfig.SENTRY_WEB_ENABLE, 
> ServerConfig.SENTRY_WEB_ENABLE_DEFAULT)) may return an NPE if conf.getBoolean 
> returns a nullBoolean (not the primitive type)
> RoleServlet at L45, use Preconditions.checkNotNull or Objects.requreNonNull 
> instead of assert
> RoleServlet at L55: String json = new Gson().toJson(roleMap);, it's better to 
> reuse the Gson instance.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  4508361b 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/RolesServlet.java
>  9ed2bf31 
> 
> 
> Diff: https://reviews.apache.org/r/69618/diff/1/
> 
> 
> Testing
> ---
> 
> builded and tested it.
> 
> 
> Thanks,
> 
> hongtaq zhao
> 
>

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

[Arjun Mishra via Review Board]

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


Ship it!




Ship It!

- Arjun Mishra


On Dec. 20, 2018, 3:45 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69586/
> ---
> 
> (Updated Dec. 20, 2018, 3:45 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/AuthorizingObjectStore.java
>  92eb1366be44bd53f57e0900634b1cb4eae6470e 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStoreBase.java
>  d015085c71822c34a3315dc884596acc8ee2421a 
>   
> 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/SentryHiveMetaStoreClient.java
>  e30a86050a23a69cb9d613ec3500a1915974ed65 
>   
> 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 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/Subject.java
>  bcd1fa2351f7e7928f5499aa5f86906640f62504 
> 
> 
> Diff: https://reviews.apache.org/r/69586/diff/3/
> 
> 
> Testing
> ---
> 
> Added unit tests for the SentryMetaStoreFilterHook.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

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

[Arjun Mishra via Review Board]

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



Sergio, seems like we are authorizing one database or one table at a time and 
then adding it to the list of filtered entities. Can we authorize them 
collectively in a single transacation?

- Arjun Mishra


On Dec. 20, 2018, 3:45 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69586/
> ---
> 
> (Updated Dec. 20, 2018, 3:45 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/AuthorizingObjectStore.java
>  92eb1366be44bd53f57e0900634b1cb4eae6470e 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStoreBase.java
>  d015085c71822c34a3315dc884596acc8ee2421a 
>   
> 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/SentryHiveMetaStoreClient.java
>  e30a86050a23a69cb9d613ec3500a1915974ed65 
>   
> 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 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/Subject.java
>  bcd1fa2351f7e7928f5499aa5f86906640f62504 
> 
> 
> Diff: https://reviews.apache.org/r/69586/diff/3/
> 
> 
> Testing
> ---
> 
> Added unit tests for the SentryMetaStoreFilterHook.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

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

[Arjun Mishra via Review Board]

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



Sergio I don't see the binding instantiated with privilege cache. Can you 
please implement it? It really speeds up performance. Thoughts?

===
HiveAuthzBinding binding = null;
try {
  binding = getHiveBindingWithPrivilegeCache(hiveAuthzBinding, 
context.getUserName());
} catch (SemanticException e) {
  // Will use the original hiveAuthzBinding
  binding = hiveAuthzBinding;
}
===

- Arjun Mishra


On Dec. 20, 2018, 3:45 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69586/
> ---
> 
> (Updated Dec. 20, 2018, 3:45 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/AuthorizingObjectStore.java
>  92eb1366be44bd53f57e0900634b1cb4eae6470e 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStoreBase.java
>  d015085c71822c34a3315dc884596acc8ee2421a 
>   
> 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/SentryHiveMetaStoreClient.java
>  e30a86050a23a69cb9d613ec3500a1915974ed65 
>   
> 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 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/Subject.java
>  bcd1fa2351f7e7928f5499aa5f86906640f62504 
> 
> 
> Diff: https://reviews.apache.org/r/69586/diff/3/
> 
> 
> Testing
> ---
> 
> Added unit tests for the SentryMetaStoreFilterHook.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

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

[Arjun Mishra via Review Board]

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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/MetastoreAuthzObjectFilter.java
Lines 184 (patched)


We should throw an AuthorizationException with message here. Otherwise the 
expected privileges won't be printed out on Hive console


- Arjun Mishra


On Dec. 20, 2018, 3:45 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69586/
> ---
> 
> (Updated Dec. 20, 2018, 3:45 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/AuthorizingObjectStore.java
>  92eb1366be44bd53f57e0900634b1cb4eae6470e 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStoreBase.java
>  d015085c71822c34a3315dc884596acc8ee2421a 
>   
> 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/SentryHiveMetaStoreClient.java
>  e30a86050a23a69cb9d613ec3500a1915974ed65 
>   
> 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 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/Subject.java
>  bcd1fa2351f7e7928f5499aa5f86906640f62504 
> 
> 
> Diff: https://reviews.apache.org/r/69586/diff/3/
> 
> 
> Testing
> ---
> 
> Added unit tests for the SentryMetaStoreFilterHook.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 69561: SENTRY-2480: Change processDropDatabase to call removeAllPaths

[Arjun Mishra via Review Board]

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

(Updated Dec. 17, 2018, 6:53 p.m.)


Review request for sentry and kalyan kumar kalvagadda.


Changes
---

Fixed test failure


Bugs: SENTRY-2480
https://issues.apache.org/jira/browse/SENTRY-2480


Repository: sentry


Description
---

Right now it calls removePaths. Remove paths has a bug that it doesn't check if 
all associated paths are deleted and the object should be deleted


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
 ab262d0a8 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestNotificationProcessor.java
 f227bb458 


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

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


Testing (updated)
---

$ mvn -f sentry-service/sentry-service-server/pom.xml test 
-Dtest=TestNotificationProcessor


Thanks,

Arjun Mishra

Review Request 69573: SENTRY-2477: When requesting for deltas check if nn seq num is 1 more than latest sequence num

[Arjun Mishra via Review Board]

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

Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
Sergio Pena.


Bugs: SENTRY-2477
https://issues.apache.org/jira/browse/SENTRY-2477


Repository: sentry


Description
---

If NN seq number and latest sentry sequence number is larger than 1 we need to 
request a full update


Diffs
-

  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
 08b16a4df 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java
 f86ce6f83 


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


Testing
---

$ mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
-Dtest=TestDBUpdateForwarder


Thanks,

Arjun Mishra

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

[Arjun Mishra via Review Board]

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


Ship it!




Ship It!

- Arjun Mishra


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

Review Request 69561: SENTRY-2480: Change processDropDatabase to call removeAllPaths

[Arjun Mishra via Review Board]

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

Review request for sentry and kalyan kumar kalvagadda.


Bugs: SENTRY-2480
https://issues.apache.org/jira/browse/SENTRY-2480


Repository: sentry


Description
---

Right now it calls removePaths. Remove paths has a bug that it doesn't check if 
all associated paths are deleted and the object should be deleted


Testing
---


Thanks,

Arjun Mishra

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

[Arjun Mishra via Review Board]

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



Also test case to cover case when an object already exist and we add a path to 
it

- Arjun Mishra


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 69087: SENTRY-2249: Enable batch insert of HMS paths in Full Snapshot.

[Arjun Mishra via Review Board]

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




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


We need an else logic hear for case when MAuthzPathsMapping object already 
exists

for (String path: paths) {
mAuthzPathsMapping.addPathToPersist(path);
}



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


We are deleting objects but not the paths associated with them. We need to 
add "mAuthzPathsMapping.deletePersistent(pm, null);" before we delete the object


- Arjun Mishra


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 69087: SENTRY-2249: Enable batch insert of HMS paths in Full Snapshot.

[Arjun Mishra via Review Board]

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



Kalyan,we need to persist MAuthzPathsMapping object at the end makepersistent. 
I tested and looks like MAuthzPathsMapping object that already exist are not 
getting updated

- Arjun Mishra


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 69087: SENTRY-2249: Enable batch insert of HMS paths in Full Snapshot.

[Arjun Mishra via Review Board]

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


Fix it, then Ship it!





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


Kalyan this log message is wrong. Please change %d to {} or use String 
format


- Arjun Mishra


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 69530: SENTRY-2476: Optimize deleting specific paths for objects

[Arjun Mishra via Review Board]

> On Dec. 11, 2018, 8:56 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3451-3455 (patched)
> > 
> >
> > Why to use two different methods instead of using only the compact? 
> > Doesn't do the same thing?

Sergio we found a case where we couldn't delete paths without knowing the 
objects. Case with 1 path multiple objects. Thats why two methods. If we detect 
a single path is associated with multiple objects we do the old approach of 
getting the objects first and then the paths. Else we delete the paths directly


> On Dec. 11, 2018, 8:56 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3485 (patched)
> > 
> >
> > I did not understand this. Why do you to read all the 
> > MAuthzPathsMapping objects? That might be a huge number considering the 
> > thousand of objects (or millions) that HMS can have and that several 
> > snapshots may be stored in the DB due to old bugs. This would read all of 
> > them.
> > 
> > Isn't it going to perform bad?

We are NOT reading all the MAuthzPathsMapping objects. We are reading all 
MAuthzPathsMapping objects for a given OBJECT NAME. So if there are multiple 
snapshots we get all entries of MAuthzPathsMapping for a given name


> On Dec. 11, 2018, 8:56 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3590 (patched)
> > 
> >
> > This method reads all MAuthzPathsMapping object to check only if there 
> > are multiple MAuthzPathsMapping relations to a set of MAuthzPath objects. 
> > 
> > Is there a way to check that with less data read from the db? Take into 
> > account that there might be thousdans of MAuthzPathsMapping.

Its a smiple query. I don't think this will impact performance


> On Dec. 11, 2018, 8:56 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3592-3595 (patched)
> > 
> >
> > This code is repeated in the Exhaustive methods. Isn't it better to 
> > pass the snapshotID as parameter instead? You can get the snapshotID once 
> > from the ...Core() method.

Ok works.


- Arjun


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


On Dec. 11, 2018, 8:16 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69530/
> ---
> 
> (Updated Dec. 11, 2018, 8:16 p.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
> 
> This affects, deleteAuthzPathsMapping, renameAuthzPathsMapping, 
> updateAuthzPathsMapping, and addAuthzPathsMappingCore methods that are known 
> at the moment
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d3 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  f5802d701 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  ca8c41610 
> 
> 
> Diff: https://reviews.apache.org/r/69530/diff/5/
> 
> 
> 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

[Arjun Mishra via Review Board]

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

(Updated Dec. 11, 2018, 8:16 p.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

This affects, deleteAuthzPathsMapping, renameAuthzPathsMapping, 
updateAuthzPathsMapping, and addAuthzPathsMappingCore methods that are known at 
the moment


Diffs (updated)
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
 d8c1061d3 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
 f5802d701 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 e2d6c85ac 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 ca8c41610 


Diff: https://reviews.apache.org/r/69530/diff/5/

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


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.

[Arjun Mishra via Review Board]

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


Ship it!




Ship It!

- Arjun Mishra


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 69530: SENTRY-2476: Optimize deleting specific paths for objects

[Arjun Mishra via Review Board]

> On Dec. 10, 2018, 9:18 p.m., kalyan kumar kalvagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3447-3470 (original), 3447-3462 (patched)
> > 
> >
> > I agree with lina's comment.
> > 
> > here is my suggested approach
> > Currently there is no filter provided while fetching, 
> > MAuthzPathsMapping. Add a new API which takes the paths as filter so that 
> > all the paths are fetches. With this, only paths match the pattern are 
> > fetches from database.
> > 
> > This solves the issue with fetching all the paths associated with an 
> > authzObj.

Kalyan, as discussed that approaach won't work as AUTHZ_PATHS_MAPPING object 
retrieved will get all references to all associated AUTHZ_PATH objects


- Arjun


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


On Dec. 11, 2018, 12:07 a.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69530/
> ---
> 
> (Updated Dec. 11, 2018, 12:07 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-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d3 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
>  f5802d701 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  ca8c41610 
> 
> 
> Diff: https://reviews.apache.org/r/69530/diff/4/
> 
> 
> 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

[Arjun Mishra via Review Board]

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

(Updated Dec. 11, 2018, 12:07 a.m.)


Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
Sergio Pena.


Changes
---

Post Lina feedback


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

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
 d8c1061d3 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java
 f5802d701 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 e2d6c85ac 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 ca8c41610 


Diff: https://reviews.apache.org/r/69530/diff/4/

Changes: https://reviews.apache.org/r/69530/diff/3-4/


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

[Arjun Mishra via Review Board]

> On Dec. 10, 2018, 4:57 p.m., Na Li wrote:
> > 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.

That would be the case with current scenario too. If you had 2 objects "tbl1", 
and "tbl2" mapped to a single path "/user/hive/warehouse/tbl", if we delete 
tbl1, we also delete the path "/user/hive/warehouse/tbl".

Let me run a test case for it


- Arjun


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


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

[Arjun Mishra via Review Board]

> On Dec. 10, 2018, 4:57 p.m., Na Li wrote:
> > 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

WE don't need to check "currentSnapshotID". Right now there is no way to 
retrieve paths for a specific snapshot Id without getting it from authz mapping 
objects.


- Arjun


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


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

[Arjun Mishra via Review Board]

> On Dec. 10, 2018, 4:57 p.m., Na Li wrote:
> > 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"

We don't need to check "currentSnapshotID". When deleting paths we can delete 
paths irrespective of the snapshot Id they belong to. Right now there is no way 
to retrieve paths for a specific snapshot Id without getting it from authz 
mapping objects.


- Arjun


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


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

[Arjun Mishra via Review Board]

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


Changes
---

Bug fixes


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

  
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/

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

[Arjun Mishra via Review Board]

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

(Updated Dec. 7, 2018, 11:26 p.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/2/


Testing (updated)
---

$ 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

[Arjun Mishra via Review Board]

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

(Updated Dec. 7, 2018, 11:25 p.m.)


Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
Sergio Pena.


Changes
---

Made changes to rename and update authz paths mapping methods as well


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

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

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


Testing
---


Thanks,

Arjun Mishra

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

[Arjun Mishra via Review Board]

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

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


Testing
---


Thanks,

Arjun Mishra

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

[Arjun Mishra via Review Board]

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

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 69415: SENTRY-2463: Revoking ALL or * should revoke any other privilege on the entity

[Arjun Mishra via Review Board]

> On Nov. 30, 2018, 5:46 p.m., kalyan kumar kalvagadda wrote:
> > I understand there is a gap here but we should be carefull in changing the 
> > current behavior.I assume, the idea here is to find a way to remove all the 
> > privileges granted to an authrozable(server/database/table)
> > 
> > Using "ALL" to soleve is not backward compatable. There could be customers 
> > who want to remove only "ALL" privilege and nothing else when "ALL" is 
> > revoked. Instead we should try to solve this with out using "ALL".

Thanks Kalyan. I'll put this in the backburner.


- Arjun


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


On Nov. 30, 2018, 5:21 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69415/
> ---
> 
> (Updated Nov. 30, 2018, 5:21 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, 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/2/
> 
> 
> 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 69415: SENTRY-2463: Revoking ALL or * should revoke any other privilege on the entity

[Arjun Mishra via Review Board]

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

(Updated Nov. 30, 2018, 5:21 p.m.)


Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
Sergio Pena.


Changes
---

Addressed Lina's comments


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

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

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


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 68547: SENTRY-2370: Create a cache of PathUpdates to send to NN

[Arjun Mishra via Review Board]

> On Sept. 28, 2018, 4:56 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
> > Lines 136-137 (patched)
> > 
> >
> > Why is the cache needed to be instantiated and passed as aparameter 
> > instead of instatiating the cache inside the PathImageRetriever and 
> > PermImageRetriever instead?
> > 
> > A retriever returns the paths, but if they're cache, then it returns 
> > the ones from the cache.
> 
> Arjun Mishra wrote:
> Sergio we haev 2 path classes PathImageRetriever, PathDeltaRetriever. 
> Even though we need cache to be in PathImageRetriever, we need the cache to 
> be visible to PathDeltaRetriever so it can be invalidated.
> 
> Sergio Pena wrote:
> Is a cache needed for deltas as well? I think the cache should be handled 
> internally on each retriever.

Yes but that is a different ticket


- Arjun


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


On Oct. 5, 2018, 7:11 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68547/
> ---
> 
> (Updated Oct. 5, 2018, 7:11 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2370
> https://issues.apache.org/jira/browse/SENTRY-2370
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When NN requests path updates from sentry and if it exceeds the time 
> threshold, on consecutive attempts sentry will attempt to fetch the full 
> update from scratch. Instead it should cache it and update the cache before 
> sending it to NN
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  08b16a4df 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java
>  f86ce6f83 
> 
> 
> Diff: https://reviews.apache.org/r/68547/diff/11/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

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

[Arjun Mishra via Review Board]

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

  
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/

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


Testing
---


Thanks,

Arjun Mishra

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

[Arjun Mishra via Review Board]

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

(Updated Nov. 27, 2018, 9:31 p.m.)


Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
Sergio Pena.


Changes
---

Post feedback


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

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

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


Testing
---


Thanks,

Arjun Mishra

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

[Arjun Mishra 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.
> 
> Na Li wrote:
> 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

It isn't since there is 1 KafkaAuthzBinding for every Kafka broker. Even if it 
was this change should be handled in a different ticket right?


- Arjun


---
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 69448: SENTRY-2464: Catch exception thrown on first reload for UpdatableCache

[Arjun Mishra 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?

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.


- Arjun


---
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 69448: SENTRY-2464: Catch exception thrown on first reload for UpdatableCache

[Arjun Mishra via Review Board]

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

(Updated Nov. 26, 2018, 6:14 p.m.)


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


Bugs: SENTRY-2464
https://issues.apache.org/jira/browse/SENTRY-2464


Repository: sentry


Description (updated)
---

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

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

[Arjun Mishra via Review Board]

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

Review request for sentry, 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, thrown an exception will 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

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

[Arjun Mishra via Review Board]

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

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 69349: SENTRY-2457: Reuse connection objects on TestConcurrentClients#testConcurrentHS2Client

[Arjun Mishra via Review Board]

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


Ship it!




Ship It!

- Arjun Mishra


On Nov. 15, 2018, 4 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69349/
> ---
> 
> (Updated Nov. 15, 2018, 4 p.m.)
> 
> 
> Review request for sentry and Arjun Mishra.
> 
> 
> Bugs: semtry-2457
> https://issues.apache.org/jira/browse/semtry-2457
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Re-use the Connection object on the 
> TestConcurrentClient#testConcurrentHS2Client method. This increase the speed 
> of the test 10s.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java
>  2da6c6b8231ff2e59c302c1ecc857f852d7a28ee 
> 
> 
> Diff: https://reviews.apache.org/r/69349/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 69260: SENTRY-2441: When MAuthzPathsMapping is deleted all associated MPaths should be deleted automatically.

[Arjun Mishra via Review Board]

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


Ship it!




Ship It!

- Arjun Mishra


On Nov. 6, 2018, 6:08 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69260/
> ---
> 
> (Updated Nov. 6, 2018, 6:08 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2441
> https://issues.apache.org/jira/browse/SENTRY-2441
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When MAuthzPathsMapping is deleted all associated MPaths should be deleted 
> automatically as MPaths are directly associated to MAuthzPathsMapping.
> 
>  
> 
> Currently when entries in MAuthzPathsMapping are deleted the FK in MPaths is 
> set to NULL and the entries are left. These elements are slate are not not 
> used and will never be cleaned.
> 
>  
> 
>  
> 
> Instead MPaths should be removed automatically associated MAuthzPathsMapping 
> entries are deleted.
> 
> 
> Diffs
> -
> 
>   
> 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 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  66db6ae9a436b9728fb3c2ebdd21167ef042f937 
> 
> 
> Diff: https://reviews.apache.org/r/69260/diff/2/
> 
> 
> Testing
> ---
> 
> Made sure that all the existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69260: SENTRY-2441: When MAuthzPathsMapping is deleted all associated MPaths should be deleted automatically.

[Arjun Mishra via Review Board]

> On Nov. 6, 2018, 5:11 p.m., Na Li wrote:
> > Can you add code in unit test to verify the path is removed? For example, 
> > there is no MPath entries whose foreign key is null or the delected 
> > authorizable entry ID.

+1


- Arjun


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


On Nov. 6, 2018, 4:59 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69260/
> ---
> 
> (Updated Nov. 6, 2018, 4:59 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2441
> https://issues.apache.org/jira/browse/SENTRY-2441
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When MAuthzPathsMapping is deleted all associated MPaths should be deleted 
> automatically as MPaths are directly associated to MAuthzPathsMapping.
> 
>  
> 
> Currently when entries in MAuthzPathsMapping are deleted the FK in MPaths is 
> set to NULL and the entries are left. These elements are slate are not not 
> used and will never be cleaned.
> 
>  
> 
>  
> 
> Instead MPaths should be removed automatically associated MAuthzPathsMapping 
> entries are deleted.
> 
> 
> Diffs
> -
> 
>   
> 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/69260/diff/1/
> 
> 
> Testing
> ---
> 
> Made sure that all the existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

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

[Arjun Mishra via Review Board]

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

(Updated Oct. 30, 2018, 5:39 p.m.)


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


Changes
---

Final diff for history


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

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

Changes: https://reviews.apache.org/r/69204/diff/3-4/


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 69204: SENTRY-2437: When granting privileges a single transaction per grant causes long delays

[Arjun Mishra via Review Board]

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

(Updated Oct. 30, 2018, 5:37 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 (updated)
-

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

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


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 69204: SENTRY-2437: When granting privileges a single transaction per grant causes long delays

[Arjun Mishra via Review Board]

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


Changes
---

Added changes to revoke methods as well


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

  
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/

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

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

[Arjun Mishra via Review Board]

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

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


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 69122: SENTRY-2432: The case of a username is ignored when determining object ownership

[Arjun Mishra via Review Board]

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


Ship it!




Ship It!

- Arjun Mishra


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

[Arjun Mishra via Review Board]

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



We could also refactor the trimAndLower() method to minimize changes. What do 
you think?

- Arjun Mishra


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

[Arjun Mishra 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?
> 
> Na Li wrote:
> It is trimmed in "MSentryUser getMSentryUserByName(final String userName, 
> final boolean throwExceptionIfNotExist) "

Ok. Got it.


- Arjun


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