----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62107/#review184613 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 81 (patched) <https://reviews.apache.org/r/62107/#comment260784> Are you sure you get your boolean logic right? Also, if we are not going to retrieve full update, why bother calling `imageRetriever.getLatestImageID();` ? Also it would be nice to log a message telling that we are rather busy at the moment, call back later, please sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 115 (patched) <https://reviews.apache.org/r/62107/#comment260785> Why are we doing this the second time? ANyway, all the comments above apply here. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 106 (patched) <https://reviews.apache.org/r/62107/#comment260783> I think this can be package-private sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 297 (patched) <https://reviews.apache.org/r/62107/#comment260779> What if this was already set? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 289 (original), 300 (patched) <https://reviews.apache.org/r/62107/#comment260780> Here we return, leaving the value loadingFullSnapshot set to true forever. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 294 (original), 305 (patched) <https://reviews.apache.org/r/62107/#comment260781> Ditto sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 220 (patched) <https://reviews.apache.org/r/62107/#comment260782> This can be simplified to return hmsFollower != null && hmsFollower.isLoadingFullSnapshot(); Regarding the null == hmsFollower vs hmsFollower == null - we had this discussion earlier. In Java you can't say hmsFollower != null && hmsFollower.isLoadingFullSnapshot(); so you are not getting any advantage in the extra checks. And for a human parser, reading null == something is much worse then something == null. We are not using this reverse notation in most other places and I suggest that we keep it this way. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java Line 22 (original), 21 (patched) <https://reviews.apache.org/r/62107/#comment260786> How do these changes relate to the issue you are fixing? Are you trying to sneak in some other change? - Alexander Kolbasov On Sept. 6, 2017, 12:42 a.m., Brian Towles wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62107/ > ----------------------------------------------------------- > > (Updated Sept. 6, 2017, 12:42 a.m.) > > > Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio > Pena, and Vadim Spector. > > > Bugs: SENTRY-1918 > https://issues.apache.org/jira/browse/SENTRY-1918 > > > Repository: sentry > > > Description > ------- > > Added fix to limit sending back of full snapshot when the HMSFollower is > pulling a full snapshot > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java > 5d744214e14d6c48194b3a0bf84d6e10070b020a > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java > 8fbc10048003ab4b8a38676e241ae0fd27d2392c > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > d4feb380fa0f3bd5f237609a107295a2d23eae3b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > d44abffc199cd46d3ab7d2230dfdb2075736a8f0 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java > 1685702c6dbc9715c8885a29a80bc68509550f0b > > > Diff: https://reviews.apache.org/r/62107/diff/1/ > > > Testing > ------- > > Unit Tests > > > Thanks, > > Brian Towles > >
