> On Sept. 5, 2017, 8:21 p.m., Alexander Kolbasov wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java > > Lines 81 (patched) > > <https://reviews.apache.org/r/62107/diff/1/?file=1816055#file1816055line86> > > > > 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
Yes the boolean logic is correct. This is the case of the current image number being greater then the image number requested. If the FullSnapshot is not being updated it then sends a full dump, if it is running it will send back the empty set which will keep retrying until the the full snapshot is done. I agree we need a log message, but it will log the same thing again and again every .5 seconds till the fullsnapshot is ready. > On Sept. 5, 2017, 8:21 p.m., Alexander Kolbasov wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java > > Lines 115 (patched) > > <https://reviews.apache.org/r/62107/diff/1/?file=1816055#file1816055line120> > > > > Why are we doing this the second time? ANyway, all the comments above > > apply here. This is a seperate case where the sequence number is <= 0 which is a full update requested. So it is handled seperatlty from the image number. > On Sept. 5, 2017, 8:21 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 106 (patched) > > <https://reviews.apache.org/r/62107/diff/1/?file=1816057#file1816057line106> > > > > I think this can be package-private True > On Sept. 5, 2017, 8:21 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 297 (patched) > > <https://reviews.apache.org/r/62107/diff/1/?file=1816057#file1816057line297> > > > > What if this was already set? It wont be set already, since the thread this is running in only has a single instance. The cleanups will be fixed, but this should be fine. > On Sept. 5, 2017, 8:21 p.m., Alexander Kolbasov wrote: > > 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/diff/1/?file=1816057#file1816057line300> > > > > Here we return, leaving the value loadingFullSnapshot set to true > > forever. Yeah missed that > On Sept. 5, 2017, 8:21 p.m., Alexander Kolbasov wrote: > > 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/diff/1/?file=1816059#file1816059line24> > > > > How do these changes relate to the issue you are fixing? Are you trying > > to sneak in some other change? Well there is no other way to get a reference to the SentryServer except through the factory. Abstracting the check for if the Full Snapshot is running allows the underlying HMSFollower or other implementation that evolves to have what ever implementation it needs, but the check to remain the same. - Brian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62107/#review184613 ----------------------------------------------------------- On Sept. 5, 2017, 7:42 p.m., Brian Towles wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62107/ > ----------------------------------------------------------- > > (Updated Sept. 5, 2017, 7:42 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 > ------- > > 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 > >
