----------------------------------------------------------- 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 > >
