> On May 4, 2017, 5:47 a.m., Na Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 322 (patched) > > <https://reviews.apache.org/r/58221/diff/23/?file=1707217#file1707217line352> > > > > it would be easier to read the code by keeping this section outside of > > previous finally block
This isn't about being easier to read. What may happen is that you get exception and the rest of the code isn't executed. The question is what code *must* be executed. > On May 4, 2017, 5:47 a.m., Na Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java > > Lines 165 (patched) > > <https://reviews.apache.org/r/58221/diff/23/?file=1707218#file1707218line165> > > > > Other functions in this class are public. Why do we want to limit it to > > just package private? Usually we do not want to make anything public unless we need to. This particular function is new and there is no need for it to be public. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58221/#review173866 ----------------------------------------------------------- On May 4, 2017, 5:48 a.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58221/ > ----------------------------------------------------------- > > (Updated May 4, 2017, 5:48 a.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar > kalvagadda, and Sergio Pena. > > > Repository: sentry > > > Description > ------- > > SENTRY-1649 move HMS follower to runServer > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > ec8676e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 132db63 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java > ce73358 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > 834ed41 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java > 1ace07c > > > Diff: https://reviews.apache.org/r/58221/diff/24/ > > > Testing > ------- > > > Thanks, > > Na Li > >
