> On June 21, 2017, 10:13 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
> > Lines 92 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754606#file1754606line92>
> >
> > There are several exceptions listed below - should they be mentioned
> > here as well?
>
> kalyan kumar kalvagadda wrote:
> All the exception are listed here. Ami missing something.
The call has
void connect()
throws IOException, InterruptedException, LoginException, MetaException {
The comment only lists MetaException
> On June 21, 2017, 10:13 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
> > Lines 174 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754606#file1754606line174>
> >
> > This is a problem with the original code - if client.close() throws an
> > exception, the kerberos context will never be shut down.
>
> kalyan kumar kalvagadda wrote:
> close on client doesn't throw an exception. Do you think it is an issue
> even then?
Hopefully it doesn't but I wouldn't be surprised if transport close throws some
unchecked exceptions.
> On June 21, 2017, 10:13 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
> > Lines 198 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754606#file1754606line198>
> >
> > Do we really need a custom exception here?
>
> kalyan kumar kalvagadda wrote:
> persistLastProcessedNotificationID and persistFullSnapShot should throw
> exceptions. If there are exceptions persisting them HMSFollower should not
> proceed futhrur.
> If we do not want to throw a custom exception we should the exception
> that they recived and the handle them in HMSFollower run where all the
> exceptions are handled as an unknown exception.
>
> I feel having custom xustom exception for spefix failure helps us handle
> them better.
What is the value in having separate exception for these two cases? We can'do
anything particularly different in these cases. It would be useful it callers
can respond differently which they can't do. We do log the reason anyway - so
what value do you see with custom exceptions?
> On June 21, 2017, 10:13 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
> > Lines 205 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754606#file1754606line205>
> >
> > I don't think it can ever be null
>
> kalyan kumar kalvagadda wrote:
> There is no harm in making sure of it.
We are not checking everything for null. If we have a contract that specifies
that the value can't be null there is no reason to check for it.
> On June 21, 2017, 10:13 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java
> > Lines 21 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754608#file1754608line21>
> >
> > Seems like other Sentry exceptions are using unique version IDs. I am
> > not sure how they ar egenerated.
>
> kalyan kumar kalvagadda wrote:
> I have done some digging into it to understand why version ID is used.
> They are used when the exceptions are serialized and de-serialized. That is
> not the case here so i used 1;
That's Ok, but it is better to be consistent - we have some exceptions that are
using real IDs and some that use 1.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review178456
-----------------------------------------------------------
On June 22, 2017, 8:28 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 22, 2017, 8:28 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li,
> Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
> 0bd0833
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java
> 0d54548
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> 8b19c88
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java
> PRE-CREATION
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
> PRE-CREATION
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> 1f7eb18
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java
> PRE-CREATION
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
> 6762de7
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
> 215f7d5
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SnapshotCreationException.java
> PRE-CREATION
>
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java
> PRE-CREATION
>
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
> 66ad2a1
>
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java
> PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/5/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added
> new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
