> On Aug. 31, 2017, 6:28 a.m., Misha Dmitriev wrote: > > 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.
This case is interesting because we sometimes use ArrayList and sometimes Set as the underlying collection, that's why I wanted to use Collections as the type here. > On Aug. 31, 2017, 6:28 a.m., Misha Dmitriev wrote: > > 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/diff/1/?file=1808325#file1808325line143> > > > > 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. This doesn't work since Map<String, Set<String>> is not compatible with Map<String, Collection<String>>. Here we know exactly that we are using sets but interfaces require collections. I don't know how to work around this. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62007/#review184238 ----------------------------------------------------------- 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 > >
