----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62107/#review184692 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 86-90 (patched) <https://reviews.apache.org/r/62107/#comment260881> This code is repeated twice. Shouldn't be good to create a private method that retrieves the full image or an empty list if one is being loaded? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 120 (patched) <https://reviews.apache.org/r/62107/#comment260880> Do we need the 'seqNum <= SEQUENCE_NUMBER_UPDATE_UNINITIALIZED' condition? If an HDFS request comes in with a seqNum = 5, then we will send a new snapshot even if one is being loaded on the HMSFollower? sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java Lines 67-69 (patched) <https://reviews.apache.org/r/62107/#comment260882> Which code path we're executing here? The condition that appears on the start that checks if a snapshot is required is not executed: if (curImgNum > imgNum) { if (SentryServiceFactory.getInstance().isFullUpdateRunning()) { LOGGER.debug( "A full update is being loaded. Delaying updating client with full image until its finished."); return Collections.emptyList(); } // In case a new HMS snapshot has been processed, then return a full paths image. LOGGER.info("A newer full update is found with image number: {}", curImgNum); return Collections.singletonList(imageRetriever.retrieveFullImage()); } curImgNum = 1 imgNum = 1 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 299 (patched) <https://reviews.apache.org/r/62107/#comment260885> Is this assertion displayed on logs as an error? Could this error happen at some point? HMSFollower is not configured to run by several threads, I think we're safe, aren't we? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java Line 25 (original), 33 (patched) <https://reviews.apache.org/r/62107/#comment260887> Is the 'instance' object overriden if we call create() more than once? I've seen code where the create() or getInstance() checks if instance is null before creating the object, if it is not null, then it justs returns the instance without creating it again. Maybe we could merge create() and getInstance() in one method? - Sergio Pena On Sept. 6, 2017, 3:52 p.m., Brian Towles wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62107/ > ----------------------------------------------------------- > > (Updated Sept. 6, 2017, 3:52 p.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 > ------- > > SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected > > 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/4/ > > > Testing > ------- > > Unit Tests > > > Thanks, > > Brian Towles > >
