----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55594/#review161908 -----------------------------------------------------------
Fix it, then Ship it! sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java (line 132) <https://reviews.apache.org/r/55594/#comment233188> Nit: if this is a one-line comment in the code (rather than javadoc), I don't see why it should take 3 lines. Can be just "// check with grant option". sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java (line 155) <https://reviews.apache.org/r/55594/#comment233189> Same as above. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java (line 215) <https://reviews.apache.org/r/55594/#comment233190> Nit: space after comma. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java (line 385) <https://reviews.apache.org/r/55594/#comment233191> Same as above - why waste a couple of lines here. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 986) <https://reviews.apache.org/r/55594/#comment233193> It's a good practice to avoid creating a new object every time we want to return an empty result. Use Collections.EMPTY_LIST instead. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1035) <https://reviews.apache.org/r/55594/#comment233194> Nit: why put this on a separate line? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1043) <https://reviews.apache.org/r/55594/#comment233195> Nit: why spread this over so many lines? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1081) <https://reviews.apache.org/r/55594/#comment233197> Nit: why add this 'result' variable, why not return query.execute... right away? - Misha Dmitriev On Jan. 17, 2017, 12:26 a.m., Alexander Kolbasov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55594/ > ----------------------------------------------------------- > > (Updated Jan. 17, 2017, 12:26 a.m.) > > > Review request for sentry, Colin Ma, Misha Dmitriev, Hao Hao, kalyan kumar > kalvagadda, Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1594 > https://issues.apache.org/jira/browse/SENTRY-1594 > > > Repository: sentry > > > Description > ------- > > SENTRY-1594 TransactionBlock should become generic > > > Diffs > ----- > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > 57b6effa1b1bbfa22fbc2114db1e502365343207 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > b7ae63430cddb81cb3235a7b04ecf95410d8604f > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionBlock.java > 7a57a9658b1b0e6b36aec2c61d187527b0adee4e > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java > 85a5326ba52fc532f3d933f7e62a352f4d4080df > > Diff: https://reviews.apache.org/r/55594/diff/ > > > Testing > ------- > > > Thanks, > > Alexander Kolbasov > >
