----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68547/#review208090 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java Line 52 (original), 54 (patched) <https://reviews.apache.org/r/68547/#comment291848> This condition is not correct. When cachePathsUpdate is not null it will always return cachePathsUpdate. cachePathsUpdate will never be updated. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java Lines 52-53 (original), 54-71 (patched) <https://reviews.apache.org/r/68547/#comment291849> General comment on the solution. From the description you are trying efficiently serve the full update request while is received immediatly after the timeout failure. If not please explain other cases you are trying to solve. 1. With this approach sentry would always store a full paths update all the time. This will increase the memory foot print of sentry. This is not good. 2. Why is thsi logic added only for Path updates but not permission updates. 3. I would prefer you differ this patch till SENTRY-2287 is fixed. Once patch for SENTRY-2287 is committed you will have place holder to reset the cache when the NN acknowledges that it received full update. That wat the cache is held only short period. - kalyan kumar kalvagadda On Aug. 28, 2018, 7:08 p.m., Arjun Mishra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68547/ > ----------------------------------------------------------- > > (Updated Aug. 28, 2018, 7:08 p.m.) > > > Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena. > > > Bugs: SENTRY-2370 > https://issues.apache.org/jira/browse/SENTRY-2370 > > > Repository: sentry > > > Description > ------- > > When NN requests path updates from sentry and if it exceeds the time > threshold, on consecutive attempts sentry will attempt to fetch the full > update from scratch. Instead it should cache it and update the cache before > sending it to NN > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java > 2b1618134 > > > Diff: https://reviews.apache.org/r/68547/diff/1/ > > > Testing > ------- > > > Thanks, > > Arjun Mishra > >
