You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Na Li <li...@cloudera.com> on 2017/05/08 18:07:30 UTC
Review Request 59058: SENTRY-1752 HMSFollower gets stuck once it
fails to process a notification event
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59058/
-----------------------------------------------------------
Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
Bugs: sentry-1752
https://issues.apache.org/jira/browse/sentry-1752
Repository: sentry
Description
-------
Catch exception in processing notification event and move on. Add unit test
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java e271ea4
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java fd97936
Diff: https://reviews.apache.org/r/59058/diff/1/
Testing
-------
add unit test
Thanks,
Na Li
Re: Review Request 59058: SENTRY-1752 HMSFollower gets stuck once it
fails to process a notification event
Posted by Na Li <li...@cloudera.com>.
> On May 8, 2017, 11:21 p.m., Alexander Kolbasov wrote:
> > I don't see how the fix would actually help - the only way for HMSFollower to make forward progress is to update notification IDs - otherwise it will process the same event over and over and continue failing.
right now, In HMSFollower constructor, we get event ID (currentEventID) from data base ("currentEventID = getLastProcessedNotificationID();") In each run(), the currentEventID in memory is advanced after processing each notification event and used to get new notifications from meta store.
This fix makes sure currentEventID advances even when having exception at processing the notification event. So it won't stuck. sentry-1669 and sentry-1726 will save currentEventID into DB after processing, and read currentEventID from DB before processing.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59058/#review174239
-----------------------------------------------------------
On May 8, 2017, 6:07 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59058/
> -----------------------------------------------------------
>
> (Updated May 8, 2017, 6:07 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: sentry-1752
> https://issues.apache.org/jira/browse/sentry-1752
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Catch exception in processing notification event and move on. Add unit test
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java e271ea4
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java fd97936
>
>
> Diff: https://reviews.apache.org/r/59058/diff/1/
>
>
> Testing
> -------
>
> add unit test
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59058: SENTRY-1752 HMSFollower gets stuck once it
fails to process a notification event
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59058/#review174239
-----------------------------------------------------------
I don't see how the fix would actually help - the only way for HMSFollower to make forward progress is to update notification IDs - otherwise it will process the same event over and over and continue failing.
- Alexander Kolbasov
On May 8, 2017, 6:07 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59058/
> -----------------------------------------------------------
>
> (Updated May 8, 2017, 6:07 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: sentry-1752
> https://issues.apache.org/jira/browse/sentry-1752
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Catch exception in processing notification event and move on. Add unit test
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java e271ea4
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java fd97936
>
>
> Diff: https://reviews.apache.org/r/59058/diff/1/
>
>
> Testing
> -------
>
> add unit test
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59058: SENTRY-1752 HMSFollower gets stuck once it
fails to process a notification event
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59058/
-----------------------------------------------------------
(Updated June 4, 2017, 1:31 p.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
Bugs: sentry-1752
https://issues.apache.org/jira/browse/sentry-1752
Repository: sentry
Description
-------
Catch exception in processing notification event and move on. Add unit test
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 74a5afb
Diff: https://reviews.apache.org/r/59058/diff/3/
Changes: https://reviews.apache.org/r/59058/diff/2-3/
Testing
-------
add unit test
Thanks,
Na Li
Re: Review Request 59058: SENTRY-1752 HMSFollower gets stuck once it
fails to process a notification event
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59058/
-----------------------------------------------------------
(Updated May 9, 2017, 5:43 p.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
Bugs: sentry-1752
https://issues.apache.org/jira/browse/sentry-1752
Repository: sentry
Description
-------
Catch exception in processing notification event and move on. Add unit test
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java e271ea4
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java fd97936
Diff: https://reviews.apache.org/r/59058/diff/2/
Changes: https://reviews.apache.org/r/59058/diff/1-2/
Testing
-------
add unit test
Thanks,
Na Li
Re: Review Request 59058: SENTRY-1752 HMSFollower gets stuck once it
fails to process a notification event
Posted by Na Li <li...@cloudera.com>.
> On May 8, 2017, 11:28 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 386 (patched)
> > <https://reviews.apache.org/r/59058/diff/1/?file=1710733#file1710733line386>
> >
> > This approach is also not efficient. Let's say there are N notifications and the HMSFollower raises exception while processing 1st notificaition. This approach stops processing rest of the notifications until the HMSFollower is run again.
>
> Na Li wrote:
> The catch clause does not throw exception, so after finally, the processing will continue for next event in the same run. If the catch clause throws the catched exception, then the behavior will be what you mentioned
the latest update does not update this file. So the comment does not apply any more.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59058/#review174241
-----------------------------------------------------------
On June 4, 2017, 1:31 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59058/
> -----------------------------------------------------------
>
> (Updated June 4, 2017, 1:31 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: sentry-1752
> https://issues.apache.org/jira/browse/sentry-1752
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Catch exception in processing notification event and move on. Add unit test
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 74a5afb
>
>
> Diff: https://reviews.apache.org/r/59058/diff/3/
>
>
> Testing
> -------
>
> add unit test
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59058: SENTRY-1752 HMSFollower gets stuck once it
fails to process a notification event
Posted by Na Li <li...@cloudera.com>.
> On May 8, 2017, 11:28 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 386 (patched)
> > <https://reviews.apache.org/r/59058/diff/1/?file=1710733#file1710733line386>
> >
> > This approach is also not efficient. Let's say there are N notifications and the HMSFollower raises exception while processing 1st notificaition. This approach stops processing rest of the notifications until the HMSFollower is run again.
The catch clause does not throw exception, so after finally, the processing will continue for next event in the same run. If the catch clause throws the catched exception, then the behavior will be what you mentioned
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59058/#review174241
-----------------------------------------------------------
On May 8, 2017, 6:07 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59058/
> -----------------------------------------------------------
>
> (Updated May 8, 2017, 6:07 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: sentry-1752
> https://issues.apache.org/jira/browse/sentry-1752
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Catch exception in processing notification event and move on. Add unit test
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java e271ea4
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java fd97936
>
>
> Diff: https://reviews.apache.org/r/59058/diff/1/
>
>
> Testing
> -------
>
> add unit test
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59058: SENTRY-1752 HMSFollower gets stuck once it
fails to process a notification event
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59058/#review174241
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 386 (patched)
<https://reviews.apache.org/r/59058/#comment247357>
This approach is also not efficient. Let's say there are N notifications and the HMSFollower raises exception while processing 1st notificaition. This approach stops processing rest of the notifications until the HMSFollower is run again.
- kalyan kumar kalvagadda
On May 8, 2017, 6:07 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59058/
> -----------------------------------------------------------
>
> (Updated May 8, 2017, 6:07 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: sentry-1752
> https://issues.apache.org/jira/browse/sentry-1752
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Catch exception in processing notification event and move on. Add unit test
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java e271ea4
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java fd97936
>
>
> Diff: https://reviews.apache.org/r/59058/diff/1/
>
>
> Testing
> -------
>
> add unit test
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59058: SENTRY-1752 HMSFollower gets stuck once it
fails to process a notification event
Posted by Na Li <li...@cloudera.com>.
> On May 8, 2017, 6:35 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 381 (patched)
> > <https://reviews.apache.org/r/59058/diff/1/?file=1710733#file1710733line381>
> >
> > Why should be throw an exception and then catch it? Why not just avoid raising an exception and log the error?
calls to SentryStore in ProcessSingleNotificationEvent could throw exception. So we have to catch exception here.
Throwing exception in ProcessSingleNotificationEvent and catching it in its caller is easy to read. We can change those places to log error and call stack and return, but it is not consistent to other code that throws exception in error condition.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59058/#review174210
-----------------------------------------------------------
On May 8, 2017, 6:07 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59058/
> -----------------------------------------------------------
>
> (Updated May 8, 2017, 6:07 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: sentry-1752
> https://issues.apache.org/jira/browse/sentry-1752
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Catch exception in processing notification event and move on. Add unit test
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java e271ea4
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java fd97936
>
>
> Diff: https://reviews.apache.org/r/59058/diff/1/
>
>
> Testing
> -------
>
> add unit test
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59058: SENTRY-1752 HMSFollower gets stuck once it
fails to process a notification event
Posted by Na Li <li...@cloudera.com>.
> On May 8, 2017, 6:35 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 381 (patched)
> > <https://reviews.apache.org/r/59058/diff/1/?file=1710733#file1710733line381>
> >
> > Why should be throw an exception and then catch it? Why not just avoid raising an exception and log the error?
>
> Na Li wrote:
> calls to SentryStore in ProcessSingleNotificationEvent could throw exception. So we have to catch exception here.
>
> Throwing exception in ProcessSingleNotificationEvent and catching it in its caller is easy to read. We can change those places to log error and call stack and return, but it is not consistent to other code that throws exception in error condition.
This file is not changed in latest update. so the comment does not apply any more.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59058/#review174210
-----------------------------------------------------------
On June 4, 2017, 1:31 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59058/
> -----------------------------------------------------------
>
> (Updated June 4, 2017, 1:31 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: sentry-1752
> https://issues.apache.org/jira/browse/sentry-1752
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Catch exception in processing notification event and move on. Add unit test
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 74a5afb
>
>
> Diff: https://reviews.apache.org/r/59058/diff/3/
>
>
> Testing
> -------
>
> add unit test
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59058: SENTRY-1752 HMSFollower gets stuck once it
fails to process a notification event
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59058/#review174210
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 381 (patched)
<https://reviews.apache.org/r/59058/#comment247329>
Why should be throw an exception and then catch it? Why not just avoid raising an exception and log the error?
- kalyan kumar kalvagadda
On May 8, 2017, 6:07 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59058/
> -----------------------------------------------------------
>
> (Updated May 8, 2017, 6:07 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: sentry-1752
> https://issues.apache.org/jira/browse/sentry-1752
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Catch exception in processing notification event and move on. Add unit test
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java e271ea4
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java fd97936
>
>
> Diff: https://reviews.apache.org/r/59058/diff/1/
>
>
> Testing
> -------
>
> add unit test
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 59058: SENTRY-1752 HMSFollower gets stuck once it
fails to process a notification event
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59058/#review174227
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 121 (patched)
<https://reviews.apache.org/r/59058/#comment247340>
You may want to remove the 'public' scope as this method is used only for testing, and unit tests exist in the same package scope. This way we avoid developers to find this method accessible when the package is imported.
Also, our coding convention is to start method names with lower case.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 379 (patched)
<https://reviews.apache.org/r/59058/#comment247341>
We should write a better error message. Example:
"An error happened while processing a HMS notification event: {EVENT_ID} {EVENT_TYPE}", e
Same thing in the 'catch (Throwable)'. We should print the the event that failed + the exception.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
Lines 174 (patched)
<https://reviews.apache.org/r/59058/#comment247342>
What causes this notification to be invalid? Can you just add a comment line with it?
- Sergio Pena
On May 8, 2017, 6:07 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59058/
> -----------------------------------------------------------
>
> (Updated May 8, 2017, 6:07 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: sentry-1752
> https://issues.apache.org/jira/browse/sentry-1752
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Catch exception in processing notification event and move on. Add unit test
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java e271ea4
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java fd97936
>
>
> Diff: https://reviews.apache.org/r/59058/diff/1/
>
>
> Testing
> -------
>
> add unit test
>
>
> Thanks,
>
> Na Li
>
>