----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62007/#review184238 -----------------------------------------------------------
In general I wonder whether instead of the old Set<> we should now use Collection<> rather than the more explicit List<>. I think using a supertype like Collection<> makes sense only if it's used for a method parameter that can receive either List or Set. Do we really have such methods? Even if there are some, I think in most other places you can replace Collection with List safely. I think the best practice suggests that it's better to be more specific about a variable type T unless this variable either really can receive different subtypes of T, or we think it may, in principle (e.g. it's better to use a Map rather than HashMap in some library method, because someone may want to pass it a LinkedHashMap, ConcurrentHashMap, etc.) But if here in certain places we very explicitly want to use ArrayLists rather than HashSets for performance reasons, I think it's better to not put the reader in any doubt and use type List rather than Collection. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java Line 96 (original), 97 (patched) <https://reviews.apache.org/r/62007/#comment260338> It would be good to both update this javadoc to explain why we don't return a Set, and change its format to the proper /** ... */ style. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java Line 142 (original), 143 (patched) <https://reviews.apache.org/r/62007/#comment260339> I don't like these type casts at all. Type casts are ok in certain cases when we really don't know in advance the actual type of the given object. But here, at a minimum, we can change the type of the 'image' parameter to 'Map<String, Set<String>>', no? Same in many other methods below. - Misha Dmitriev On Aug. 31, 2017, 1:38 a.m., Alexander Kolbasov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62007/ > ----------------------------------------------------------- > > (Updated Aug. 31, 2017, 1:38 a.m.) > > > Review request for sentry, Arjun Mishra, Brian Towles, Misha Dmitriev, Na Li, > Sergio Pena, Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1909 > https://issues.apache.org/jira/browse/SENTRY-1909 > > > Repository: sentry > > > Description > ------- > > SENTRY-1909 Improvements for memory usage when full path snapshot is sent > from Sentry to NN > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java > 898c7bec2e35e6f1424478666282ba78222da709 > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java > 20b3e108cc976207a3809bc6a214a34e10788200 > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java > e403d7cca5dca5c39785490f92e562dc9a3b4daf > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java > fee5279a7cd3d60df4e60a5f25d2e8604c37d04b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PathsImage.java > 409a557fe89d85df888c1f16b3f5aacdd9aa96b0 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 593b92f96b47f959266280bce3d0809f6a80c362 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java > 760a2b55e98ac1fb305666cbbedfba11d26f2fcc > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java > 2cd18ea9d2ca40b5f46aaafde532fb3bf9769c07 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java > 4a8fb953ce486e1aeb1042884de56872b5539cd0 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 3da6a4e998d36cada7e9ea77285b11f07cea5f25 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateInitializer.java > 9d7bddd339513180e627286d8a749b7814c824f2 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java > 7deccb064fd75b39af779796d9e95caf9c718b32 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java > f56384a99e128b3e9880cbc5692107d61f2f500f > > > Diff: https://reviews.apache.org/r/62007/diff/1/ > > > Testing > ------- > > > Thanks, > > Alexander Kolbasov > >
