----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68547/#review209184 -----------------------------------------------------------
The same PathImageRetriever feedback applies to PermissionImageRetriever. Btw, while looking at the code completely, I think it makes more sense to keep the cache in the DBUpdateForwarder class. That class handles the seqNum and imgNum that can be used to make your logic of whether fetch a new snapshot or just return the cache. Also the code will be re-used for paths and permissions. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ImageRetriever.java Lines 65 (patched) <https://reviews.apache.org/r/68547/#comment293510> Seems this method is used only internally on each class. Is it needed to be part of the interface and public? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Line 131 (original), 131 (patched) <https://reviews.apache.org/r/68547/#comment293509> Does this 'Clear cache if present' help for supportability? How do you know the cache was free or not? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 132 (patched) <https://reviews.apache.org/r/68547/#comment293508> Leave a comment on this call why you're releasing the cache from memory, and why at this point of the call. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java Lines 44 (patched) <https://reviews.apache.org/r/68547/#comment293511> Code convention: Need a space between the variable type and name. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java Lines 62 (patched) <https://reviews.apache.org/r/68547/#comment293512> Code convention: if () instead of if() sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java Lines 65 (patched) <https://reviews.apache.org/r/68547/#comment293522> Could you call clearCache() before creating another SoftReference? Just to make sure the cache is cleared and can be claimed by GC. We want to make sure there are not hard references leaked that causes a memory leak. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java Lines 79 (patched) <https://reviews.apache.org/r/68547/#comment293520> Code convention: If () instead of if() sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java Lines 80 (patched) <https://reviews.apache.org/r/68547/#comment293519> The SoftReference object has a clear(). I think we should call it too before setting the variable to null. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java Lines 97-107 (patched) <https://reviews.apache.org/r/68547/#comment293521> Why do we need the sequence# and image# here? Those are already checked in the DbUpdateForward class. I think the cache invalidation here should be done by looking at the current snapshot ID on the SentryStore and comparing it with the one store in the cache. - Sergio Pena On Oct. 2, 2018, 9:57 p.m., Arjun Mishra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68547/ > ----------------------------------------------------------- > > (Updated Oct. 2, 2018, 9:57 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 > 3532ef33d > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java > 443434127 > > 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/9/ > > > Testing > ------- > > > Thanks, > > Arjun Mishra > >
