You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Sergio Pena <se...@cloudera.com> on 2017/09/06 22:50:32 UTC
Review Request 62136: SENTRY-1928: HMSFollower should close HMS
connections when an error to HMS occurs
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62136/
-----------------------------------------------------------
Review request for sentry, Alexander Kolbasov and Vamsee Yarlagadda.
Bugs: sentry-1928
https://issues.apache.org/jira/browse/sentry-1928
Repository: sentry
Description
-------
The patch is closing the HMS connections on the HiveNotificationFetcher if any Exception occurrs. The HMSFollower has also code to close the connection on any Exception, but I do it for legacy reasons as I think it is not required anymore. The HMSFollower needs some refactoring, though.
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d4feb380fa0f3bd5f237609a107295a2d23eae3b
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 4d329921bb4bc540992084687726155cb0373f0f
Diff: https://reviews.apache.org/r/62136/diff/1/
Testing
-------
Thanks,
Sergio Pena
Re: Review Request 62136: SENTRY-1928: HMSFollower should close HMS
connections when an error to HMS occurs
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62136/#review184752
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 191 (original), 191 (patched)
<https://reviews.apache.org/r/62136/#comment260957>
Since we are not doing anything fancy now, should we just remove this and have a simple catch-all?
- Alexander Kolbasov
On Sept. 6, 2017, 10:50 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62136/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2017, 10:50 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Vamsee Yarlagadda.
>
>
> Bugs: sentry-1928
> https://issues.apache.org/jira/browse/sentry-1928
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The patch is closing the HMS connections on the HiveNotificationFetcher if any Exception occurrs. The HMSFollower has also code to close the connection on any Exception, but I do it for legacy reasons as I think it is not required anymore. The HMSFollower needs some refactoring, though.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d4feb380fa0f3bd5f237609a107295a2d23eae3b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 4d329921bb4bc540992084687726155cb0373f0f
>
>
> Diff: https://reviews.apache.org/r/62136/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 62136: SENTRY-1928: HMSFollower should close HMS
connections when an error to HMS occurs
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62136/#review184771
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Kolbasov
On Sept. 7, 2017, 1:24 a.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62136/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2017, 1:24 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Vamsee Yarlagadda.
>
>
> Bugs: sentry-1928
> https://issues.apache.org/jira/browse/sentry-1928
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The patch is closing the HMS connections on the HiveNotificationFetcher if any Exception occurrs. The HMSFollower has also code to close the connection on any Exception, but I do it for legacy reasons as I think it is not required anymore. The HMSFollower needs some refactoring, though.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d4feb380fa0f3bd5f237609a107295a2d23eae3b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 4d329921bb4bc540992084687726155cb0373f0f
>
>
> Diff: https://reviews.apache.org/r/62136/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 62136: SENTRY-1928: HMSFollower should close HMS
connections when an error to HMS occurs
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62136/#review184767
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 188-202 (original), 188-196 (patched)
<https://reviews.apache.org/r/62136/#comment260971>
Just collapse to a single catch?
- Vamsee Yarlagadda
On Sept. 7, 2017, 1:24 a.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62136/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2017, 1:24 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Vamsee Yarlagadda.
>
>
> Bugs: sentry-1928
> https://issues.apache.org/jira/browse/sentry-1928
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The patch is closing the HMS connections on the HiveNotificationFetcher if any Exception occurrs. The HMSFollower has also code to close the connection on any Exception, but I do it for legacy reasons as I think it is not required anymore. The HMSFollower needs some refactoring, though.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d4feb380fa0f3bd5f237609a107295a2d23eae3b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 4d329921bb4bc540992084687726155cb0373f0f
>
>
> Diff: https://reviews.apache.org/r/62136/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 62136: SENTRY-1928: HMSFollower should close HMS
connections when an error to HMS occurs
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62136/
-----------------------------------------------------------
(Updated Sept. 7, 2017, 1:37 a.m.)
Review request for sentry, Alexander Kolbasov and Vamsee Yarlagadda.
Bugs: sentry-1928
https://issues.apache.org/jira/browse/sentry-1928
Repository: sentry
Description
-------
The patch is closing the HMS connections on the HiveNotificationFetcher if any Exception occurrs. The HMSFollower has also code to close the connection on any Exception, but I do it for legacy reasons as I think it is not required anymore. The HMSFollower needs some refactoring, though.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d4feb380fa0f3bd5f237609a107295a2d23eae3b
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 4d329921bb4bc540992084687726155cb0373f0f
Diff: https://reviews.apache.org/r/62136/diff/3/
Changes: https://reviews.apache.org/r/62136/diff/2-3/
Testing
-------
Thanks,
Sergio Pena
Re: Review Request 62136: SENTRY-1928: HMSFollower should close HMS
connections when an error to HMS occurs
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62136/#review184770
-----------------------------------------------------------
Ship it!
Ship It!
- Vamsee Yarlagadda
On Sept. 7, 2017, 1:24 a.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62136/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2017, 1:24 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Vamsee Yarlagadda.
>
>
> Bugs: sentry-1928
> https://issues.apache.org/jira/browse/sentry-1928
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The patch is closing the HMS connections on the HiveNotificationFetcher if any Exception occurrs. The HMSFollower has also code to close the connection on any Exception, but I do it for legacy reasons as I think it is not required anymore. The HMSFollower needs some refactoring, though.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d4feb380fa0f3bd5f237609a107295a2d23eae3b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 4d329921bb4bc540992084687726155cb0373f0f
>
>
> Diff: https://reviews.apache.org/r/62136/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 62136: SENTRY-1928: HMSFollower should close HMS
connections when an error to HMS occurs
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62136/
-----------------------------------------------------------
(Updated Sept. 7, 2017, 1:24 a.m.)
Review request for sentry, Alexander Kolbasov and Vamsee Yarlagadda.
Bugs: sentry-1928
https://issues.apache.org/jira/browse/sentry-1928
Repository: sentry
Description
-------
The patch is closing the HMS connections on the HiveNotificationFetcher if any Exception occurrs. The HMSFollower has also code to close the connection on any Exception, but I do it for legacy reasons as I think it is not required anymore. The HMSFollower needs some refactoring, though.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d4feb380fa0f3bd5f237609a107295a2d23eae3b
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 4d329921bb4bc540992084687726155cb0373f0f
Diff: https://reviews.apache.org/r/62136/diff/2/
Changes: https://reviews.apache.org/r/62136/diff/1-2/
Testing
-------
Thanks,
Sergio Pena
Re: Review Request 62136: SENTRY-1928: HMSFollower should close HMS
connections when an error to HMS occurs
Posted by Sergio Pena <se...@cloudera.com>.
> On Sept. 6, 2017, 11:37 p.m., Vamsee Yarlagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java
> > Lines 98 (patched)
> > <https://reviews.apache.org/r/62136/diff/1/?file=1816684#file1816684line100>
> >
> > Why can't we leave this to HMSFollower to handle this?
I think the HiveNotificationFetcher is the best place to close a connection in case of errors so that other callers do not have to catch exceptions and close connections. My thinking of the HiveNotificationFetcher is that it can be also used by the SentryHMSClient to consolidate notifications when fetching a new snapshot. Also, I am thinkin on refactor the SentryHMSClient to keep te context of snapshots and add the close() internally as well. This way we don't depend on the caller (like HMSFollower) to close connections if something is wrong.
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62136/#review184757
-----------------------------------------------------------
On Sept. 7, 2017, 1:24 a.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62136/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2017, 1:24 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Vamsee Yarlagadda.
>
>
> Bugs: sentry-1928
> https://issues.apache.org/jira/browse/sentry-1928
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The patch is closing the HMS connections on the HiveNotificationFetcher if any Exception occurrs. The HMSFollower has also code to close the connection on any Exception, but I do it for legacy reasons as I think it is not required anymore. The HMSFollower needs some refactoring, though.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d4feb380fa0f3bd5f237609a107295a2d23eae3b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 4d329921bb4bc540992084687726155cb0373f0f
>
>
> Diff: https://reviews.apache.org/r/62136/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 62136: SENTRY-1928: HMSFollower should close HMS
connections when an error to HMS occurs
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62136/#review184757
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java
Lines 98 (patched)
<https://reviews.apache.org/r/62136/#comment260964>
Why can't we leave this to HMSFollower to handle this?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java
Lines 186 (patched)
<https://reviews.apache.org/r/62136/#comment260965>
Same here.
- Vamsee Yarlagadda
On Sept. 6, 2017, 10:50 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62136/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2017, 10:50 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Vamsee Yarlagadda.
>
>
> Bugs: sentry-1928
> https://issues.apache.org/jira/browse/sentry-1928
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The patch is closing the HMS connections on the HiveNotificationFetcher if any Exception occurrs. The HMSFollower has also code to close the connection on any Exception, but I do it for legacy reasons as I think it is not required anymore. The HMSFollower needs some refactoring, though.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d4feb380fa0f3bd5f237609a107295a2d23eae3b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 4d329921bb4bc540992084687726155cb0373f0f
>
>
> Diff: https://reviews.apache.org/r/62136/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergio Pena
>
>