> On June 20, 2017, 11:12 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java > > Lines 206 (patched) > > <https://reviews.apache.org/r/59508/diff/4/?file=1754606#file1754606line206> > > > > If the fetchFullUpdate() snapshot ignores the failure, why we have to > > throw an exception here? > > > > This might confuse users when there are empty snapshots, right? Isn't > > better just to ignore the error and return an empty map list as well? > > kalyan kumar kalvagadda wrote: > if the shapshot has no entries, HMSFOllower should not proceed futhur and > just return.
Understand, but having an empty snapshot on HMS does not mean an error happened. Throwing exceptions should be done on special cases when the caller must be notified about an error during the code execution. Also, throwing exceptions is expensive, so having a function to throw an error just to notify the caller there was nothing to do then it looks like a bad design. The HMSFollower can handle empty snapshots, so why not returning an empty snapshot? > On June 20, 2017, 11:12 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Line 648 (original), 270 (patched) > > <https://reviews.apache.org/r/59508/diff/4/?file=1754607#file1754607line698> > > > > Couldn't be this a IOException? It is attempting to persist > > (write/output) to a DB, so it looks like IOException make sense. > > kalyan kumar kalvagadda wrote: > failing to write to sentry could fail for a lot reason, throwing I/O > exception doesn't seem to be correct. Yes, but the HMSFollowerPersistenceException and IOException are not different. Both are saying a writing failure happend and has a message error. I am trying to avoid a new exception class if it is not needed by anyone else. Custom exceptions are useful to notify the caller what really happened, and they can have extra information about the error, but in this refactor class, i don't see what the HMSFollower can do different with a custom exception. See https://blogs.msdn.microsoft.com/jaredpar/2008/10/20/custom-exceptions-when-should-you-create-them/ > On June 20, 2017, 11:12 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java > > Lines 96 (patched) > > <https://reviews.apache.org/r/59508/diff/4/?file=1754610#file1754610line96> > > > > Remove space before the parethesis. "setAuthzConf (Configuration conf)" > > > > Btw, if this method exists now, should we call it from the constructor? > > Seems we are duplicating code here. > > kalyan kumar kalvagadda wrote: > I have added it for testing purposes only. If it is re-used in > constrctor, it can not be documented as test only function. > it can be wrongly used later. It doesn't make sense. @VisibleForTesting can be still used if you call this function from the constructor. Are ther compilation errors if we do? Also, I don't understand the method. It gets a boolean value from the authzConf object, and IF it is true, then it returns true? I think Boolean.parseBoolean() does that for you, doesn't it? this function is ideanticall from what part of the constructor does. I still think that even if we use code only for testing, if it is part of the private methods, then it should be used. > On June 20, 2017, 11:12 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java > > Lines 148 (patched) > > <https://reviews.apache.org/r/59508/diff/4/?file=1754610#file1754610line150> > > > > Shouldn't this be a warning instead of error? Error implies that > > something bad happened on the code. Here, we're just ignoring these invalid > > requests. A warning is more appropiate. > > > > The same for the rest of the notifications requests. > > kalyan kumar kalvagadda wrote: > Sentry code doesn't use warn log level much. I was not sure when would it > appropariate to log it as a warning. What do you mean Sentry doesn't use warn logs? This is part of the log4j interface, isn't it? - Sergio ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59508/#review178434 ----------------------------------------------------------- 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 > >
