> On Aug. 23, 2017, 12:50 a.m., Alexander Kolbasov wrote: > > Overall looks good. This are partial comments - I still need more time to > > go through all the changes. These comments are all nits. > > > > Can you add a block comment at the top of HMSPaths.java explaining the > > trickery and the motivation for using trees rather then HashSet.
As we previously discussed in the e-mail: I myself am actually not 100% sure that we need TreeSets here. It would be good if a Sentry expert investigated this. > On Aug. 23, 2017, 12:50 a.m., Alexander Kolbasov wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java > > Line 166 (original), 166 (patched) > > <https://reviews.apache.org/r/61827/diff/1/?file=1801885#file1801885line166> > > > > Do we need a List here or we can use Collection? We just use it to walk > > entries. Currently only Lists are used as arguments to this method, but in principle, having a Collection here gives us more flexibility. Changed. > On Aug. 23, 2017, 12:50 a.m., Alexander Kolbasov wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java > > Lines 234 (patched) > > <https://reviews.apache.org/r/61827/diff/1/?file=1801885#file1801885line234> > > > > Didn't you insert the element already on line 229 above? No, in line 229 I created a TreeSet and initialized it with a single element (auth object) that was previosly referenced directly by the authzObjs field. And here I add a second element (passed to this method as an argument) to this set. > On Aug. 23, 2017, 12:50 a.m., Alexander Kolbasov wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java > > Line 502 (original), 530 (patched) > > <https://reviews.apache.org/r/61827/diff/1/?file=1801885#file1801885line534> > > > > This can be package-private Done. > On Aug. 23, 2017, 12:50 a.m., Alexander Kolbasov wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java > > Lines 532 (patched) > > <https://reviews.apache.org/r/61827/diff/1/?file=1801885#file1801885line536> > > > > It may be easier to read (and make SonarLint happy) if you extract the > > ternary operation in a seperate statement. We don't need to save memory on > > Java lines :-) Ok, replaced with if-else statements. Indeed this is not much longer and much easier to read. > On Aug. 23, 2017, 12:50 a.m., Alexander Kolbasov wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java > > Lines 539 (patched) > > <https://reviews.apache.org/r/61827/diff/1/?file=1801885#file1801885line543> > > > > Same comment here about ternary op. Changed. - Misha ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61827/#review183559 ----------------------------------------------------------- On Aug. 22, 2017, 11:02 p.m., Misha Dmitriev wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61827/ > ----------------------------------------------------------- > > (Updated Aug. 22, 2017, 11:02 p.m.) > > > Review request for sentry, Alexander Kolbasov, Brian Towles, and Vadim > Spector. > > > Bugs: SENTRY-1892 > https://issues.apache.org/jira/browse/SENTRY-1892 > > > Repository: sentry > > > Description > ------- > > SENTRY-1892: Reduce memory consumption of HMSPath and TPathEntry > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathEntry.java > 2a89368208bcef5b537cb0c2d59fd14b1735f435 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java > 6e71561e36d7f235afb14961299bfc23c03607a6 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsDumper.java > bf6c3dec35c7d22b9bd0e3f863925f7ff3e94515 > > sentry-hdfs/sentry-hdfs-common/src/main/resources/sentry_hdfs_service.thrift > 22dea02ea003530513d7fb270eb0ca0ce38957e9 > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java > a0d7bdcd256a17558178ebc5238b1f3da9fa5e9b > > > Diff: https://reviews.apache.org/r/61827/diff/1/ > > > Testing > ------- > > Ran 'mvn install' locally, all tests passed. > > > Thanks, > > Misha Dmitriev > >
