> On Aug. 29, 2018, 1:28 p.m., kalyan kumar kalvagadda wrote: > > 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/diff/1/?file=2078642#file2078642line54> > > > > 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. > > Arjun Mishra wrote: > Kalyan, agreed that it would be good to clear the cache once NN > acknoledges the receipt of udpates, but that could be worked on as a part of > a different ticket. I think this will significantly help with issues where NN > request timeout triggers another fetch from Sentry Db. The increase to heap > size will be needed but the benefits outway the problem. Let me know your > thoughts. > > I have added Perm Update cache too. I can also add a configuration to > enable path or perm caching > > kalyan kumar kalvagadda wrote: > We need to really carefull before introducing caching. When there we know > that higher timeout value avaoid this failure there is no hurry in putting a > solution that increases the memory foot print and other potential bugs.
Thanks Kalyan. The higher tmeout fixes the issue, but the timeout threshold is arbitrary and may take several iterations to get it right. Also its hard to evaluate what the timeout should be based on HMS size. In any case I will wait for your patch so the cache can be invalidated - Arjun ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68547/#review208090 ----------------------------------------------------------- On Aug. 29, 2018, 7:04 p.m., Arjun Mishra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68547/ > ----------------------------------------------------------- > > (Updated Aug. 29, 2018, 7:04 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-common/src/main/java/org/apache/sentry/hdfs/ImageRetriever.java > 11b75411d > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java > 08b16a4df > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java > 2b1618134 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java > f3a2d5028 > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java > f86ce6f83 > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java > fb42b279a > > > Diff: https://reviews.apache.org/r/68547/diff/2/ > > > Testing > ------- > > > Thanks, > > Arjun Mishra > >
