Re: Review Request 68444: SENTRY-2318: Sentry listener should log the failure if grant/revoke of owner privilege fails

[Sergio Pena via Review Board]

> On Aug. 21, 2018, 7:52 p.m., Sergio Pena wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
> > Lines 265-268 (original), 265-268 (patched)
> > 
> >
> > Agree with Lina. We cannot throw an exception in HMS as the listener is 
> > run after HMS already committed the operation. This may cause HMS to retry 
> > the commit .
> 
> kalyan kumar kalvagadda wrote:
> Just looked at the HMS code closely. This exception will cause HMS to 
> retry. This is wrong behavior in HMS for a failure in Post Event Listener.

HMS should not throw the exception back to HS2 for post-event listeners, 
otherwise the DDL operations will be committed with errors.


- Sergio


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


On Aug. 21, 2018, 5:45 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68444/
> ---
> 
> (Updated Aug. 21, 2018, 5:45 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2318
> https://issues.apache.org/jira/browse/SENTRY-2318
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> If the grant/revoke of owner privilege fails for any reason it should logged 
> be as an error in the hive logs otherwise this will go un-noticed.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
>  c37f3706092f2239e45e1cef2f31cb8ab1c886f5 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java
>  98de8e36adfd790996aba8e61a164d85e80bc006 
> 
> 
> Diff: https://reviews.apache.org/r/68444/diff/1/
> 
> 
> Testing
> ---
> 
> Updated the tests to cover the change.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68444: SENTRY-2318: Sentry listener should log the failure if grant/revoke of owner privilege fails

[kalyan kumar kalvagadda via Review Board]

> On Aug. 21, 2018, 7:52 p.m., Sergio Pena wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
> > Lines 265-268 (original), 265-268 (patched)
> > 
> >
> > Agree with Lina. We cannot throw an exception in HMS as the listener is 
> > run after HMS already committed the operation. This may cause HMS to retry 
> > the commit .

Just looked at the HMS code closely. This exception will cause HMS to retry. 
This is wrong behavior in HMS for a failure in Post Event Listener.


- kalyan kumar


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


On Aug. 21, 2018, 5:45 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68444/
> ---
> 
> (Updated Aug. 21, 2018, 5:45 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2318
> https://issues.apache.org/jira/browse/SENTRY-2318
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> If the grant/revoke of owner privilege fails for any reason it should logged 
> be as an error in the hive logs otherwise this will go un-noticed.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
>  c37f3706092f2239e45e1cef2f31cb8ab1c886f5 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java
>  98de8e36adfd790996aba8e61a164d85e80bc006 
> 
> 
> Diff: https://reviews.apache.org/r/68444/diff/1/
> 
> 
> Testing
> ---
> 
> Updated the tests to cover the change.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68444: SENTRY-2318: Sentry listener should log the failure if grant/revoke of owner privilege fails

[Sergio Pena via Review Board]

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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Lines 265-268 (original), 265-268 (patched)


Agree with Lina. We cannot throw an exception in HMS as the listener is run 
after HMS already committed the operation. This may cause HMS to retry the 
commit .


- Sergio Pena


On Aug. 21, 2018, 5:45 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68444/
> ---
> 
> (Updated Aug. 21, 2018, 5:45 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2318
> https://issues.apache.org/jira/browse/SENTRY-2318
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> If the grant/revoke of owner privilege fails for any reason it should logged 
> be as an error in the hive logs otherwise this will go un-noticed.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
>  c37f3706092f2239e45e1cef2f31cb8ab1c886f5 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java
>  98de8e36adfd790996aba8e61a164d85e80bc006 
> 
> 
> Diff: https://reviews.apache.org/r/68444/diff/1/
> 
> 
> Testing
> ---
> 
> Updated the tests to cover the change.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68444: SENTRY-2318: Sentry listener should log the failure if grant/revoke of owner privilege fails

[Na Li via Review Board]

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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Line 268 (original), 268 (patched)


What is impact to throw this exception to HMS? 

In 
https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java#L1068,

There is no catch exception for each listener call. Throwing exception in 
sentry post event listener will cause following listener not called, and any 
other code after in finally not called. 

Can we accept this behavior?


- Na Li


On Aug. 21, 2018, 5:45 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68444/
> ---
> 
> (Updated Aug. 21, 2018, 5:45 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Bugs: SENTRY-2318
> https://issues.apache.org/jira/browse/SENTRY-2318
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> If the grant/revoke of owner privilege fails for any reason it should logged 
> be as an error in the hive logs otherwise this will go un-noticed.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
>  c37f3706092f2239e45e1cef2f31cb8ab1c886f5 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java
>  98de8e36adfd790996aba8e61a164d85e80bc006 
> 
> 
> Diff: https://reviews.apache.org/r/68444/diff/1/
> 
> 
> Testing
> ---
> 
> Updated the tests to cover the change.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Review Request 68444: SENTRY-2318: Sentry listener should log the failure if grant/revoke of owner privilege fails

[kalyan kumar kalvagadda via Review Board]

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

Review request for sentry, Na Li and Sergio Pena.


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


Repository: sentry


Description
---

If the grant/revoke of owner privilege fails for any reason it should logged be 
as an error in the hive logs otherwise this will go un-noticed.


Diffs
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
 c37f3706092f2239e45e1cef2f31cb8ab1c886f5 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java
 98de8e36adfd790996aba8e61a164d85e80bc006 


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


Testing
---

Updated the tests to cover the change.


Thanks,

kalyan kumar kalvagadda