Re: Review Request 65768: SENTRY-2144: Table Rename should allow database name change

[Na Li via Review Board]

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

(Updated March 7, 2018, 10:39 p.m.)


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


Repository: sentry


Description
---

fix the bug that moves privilege to wrong table when DB name changes in alter 
table rename


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 7a31a01 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
 c30d982 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 b410027 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java
 c6be80d 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
 5fe6625 


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

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


Testing
---

unit test succeeded


Thanks,

Na Li

Re: Review Request 65945: SENTRY-2164: Convert uses of TransactionBlock to lambdas

[Steve Moist via Review Board]

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


Ship it!




Ship It!

- Steve Moist


On March 7, 2018, 7:14 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65945/
> ---
> 
> (Updated March 7, 2018, 7:14 a.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2164
> https://issues.apache.org/jira/browse/SENTRY-2164
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2164: Convert uses of TransactionBlock to lambdas
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
>  4cb46abc52985b6af65ef06e4c14ae4bf92bec0c 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a31a016f6867284446f685e59b1377ae44e970b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionBlock.java
>  c7b19cee9e5819bea56debcb5091d92a4cba9019 
> 
> 
> Diff: https://reviews.apache.org/r/65945/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 65953: SENTRY-2165: NotificationProcesser processAlterTable method has logs wrongly flagged as ERROR when they should be INFO

[Steve Moist via Review Board]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
Line 388 (original), 388 (patched)


Description says it should be info, why is it warn?


- Steve Moist


On March 7, 2018, 5:03 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65953/
> ---
> 
> (Updated March 7, 2018, 5:03 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> "Alter table event has incomplete information" and "Alter table notification 
> ignored as neither name nor location has changed" are logged as ERROR. This 
> can flood the logs and be a distraction to analyzing actual issues. These 
> should be set at INFO level
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
>  94a0b0f12 
> 
> 
> Diff: https://reviews.apache.org/r/65953/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

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

[Steve Moist via Review Board]

> On March 1, 2018, 5:39 p.m., Steve Moist wrote:
> > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
> > Lines 140 (patched)
> > 
> >
> > I don't find stating illegargumentexception to be helpful.  What kind 
> > of illegal argument woould trigger this exception to happen?
> 
> Arjun Mishra wrote:
> See KeyValue class line 37

I mean more express the exception message in a more user friendly way.  What is 
it trying to accomplish here, and why are we specifically logging it as a IAE 
when we could just collapse all the messages into single catch(Exception e) and 
log it there.


- Steve


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


On March 1, 2018, 6:19 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65866/
> ---
> 
> (Updated March 1, 2018, 6:19 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar 
> kalvagadda, Liam Sargent, Na Li, Steve Moist, Sergio Pena, Vadim Spector, and 
> Xinran Tinney.
> 
> 
> 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-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeFactory.java
>  2f8296bfa 
>   
> sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPrivilegeFactory.java
>  d338f0edd 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
>  a9b98f319 
> 
> 
> Diff: https://reviews.apache.org/r/65866/diff/2/
> 
> 
> 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
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 65768: SENTRY-2144: Table Rename should allow database name change

[Na Li via Review Board]

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

(Updated March 7, 2018, 7:16 p.m.)


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


Repository: sentry


Description
---

fix the bug that moves privilege to wrong table when DB name changes in alter 
table rename


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 7a31a01 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 b410027 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
 5fe6625 


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

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


Testing
---

unit test succeeded


Thanks,

Na Li

Re: Review Request 65953: SENTRY-2165: NotificationProcesser processAlterTable method has logs wrongly flagged as ERROR when they should be INFO

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On March 7, 2018, 5:03 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65953/
> ---
> 
> (Updated March 7, 2018, 5:03 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> "Alter table event has incomplete information" and "Alter table notification 
> ignored as neither name nor location has changed" are logged as ERROR. This 
> can flood the logs and be a distraction to analyzing actual issues. These 
> should be set at INFO level
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
>  94a0b0f12 
> 
> 
> Diff: https://reviews.apache.org/r/65953/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Review Request 65953: SENTRY-2165: NotificationProcesser processAlterTable method has logs wrongly flagged as ERROR when they should be INFO

[Arjun Mishra via Review Board]

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

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


Repository: sentry


Description
---

"Alter table event has incomplete information" and "Alter table notification 
ignored as neither name nor location has changed" are logged as ERROR. This can 
flood the logs and be a distraction to analyzing actual issues. These should be 
set at INFO level


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java
 94a0b0f12 


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


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 65945: SENTRY-2164: Convert uses of TransactionBlock to lambdas

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On March 7, 2018, 7:14 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65945/
> ---
> 
> (Updated March 7, 2018, 7:14 a.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2164
> https://issues.apache.org/jira/browse/SENTRY-2164
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2164: Convert uses of TransactionBlock to lambdas
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
>  4cb46abc52985b6af65ef06e4c14ae4bf92bec0c 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a31a016f6867284446f685e59b1377ae44e970b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionBlock.java
>  c7b19cee9e5819bea56debcb5091d92a4cba9019 
> 
> 
> Diff: https://reviews.apache.org/r/65945/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>