----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68547/#review209315 -----------------------------------------------------------
Did you look at the Guava cache? Read this part of using soft values in the Guava cache (btw, it has invalidate vs cleanUp methods which means different things): http://blog.jessitron.com/2011/10/keeping-your-cache-down-to-size.html I was looking at the Guava cache and it will be valuable resource for using it in this cache instead of doing our own thing. It is much simpler and less prone to errors. Really useful. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 139 (patched) <https://reviews.apache.org/r/68547/#comment293627> If imageCache.get() is not null, then it might be null after the if condition. The reason is GC could remove it from memory if it is not a hard-reference. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 140-145 (patched) <https://reviews.apache.org/r/68547/#comment293628> Invalidate or cleanUp? I like to separate the verbs. Invalidate means the current cache is not valid and it will be replaced (but not necessary removed from memory). Clean is what is says, clean from memory. Also, I think we should see the future of HDFS Federation. We can get a benefit if we avoid cleaning the cache when multiple NN request images from Sentry. This way Sentry will return the cache image. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Line 159 (original), 216-220 (patched) <https://reviews.apache.org/r/68547/#comment293630> In the if() above, if imageNum != than the cache, then that will return null. When is this if going to be valid? imageNum will be teh same than cache. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 222 (patched) <https://reviews.apache.org/r/68547/#comment293631> Just check if they're different. It is a simple case. A cache is not more valid if the seq num is different (higher or lower, it does not matter). This logic is assumming HDFS will never request a lower value. We should not assume that. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 248-266 (patched) <https://reviews.apache.org/r/68547/#comment293629> Is there a way to mock the cache objects instead of having new methods to get its internal value? Or mock the imageRetriever and verify it was called when a cache is not valid. - Sergio Pena On Oct. 5, 2018, 7:11 p.m., Arjun Mishra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68547/ > ----------------------------------------------------------- > > (Updated Oct. 5, 2018, 7:11 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/DBUpdateForwarder.java > 08b16a4df > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java > f86ce6f83 > > > Diff: https://reviews.apache.org/r/68547/diff/11/ > > > Testing > ------- > > > Thanks, > > Arjun Mishra > >
