----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56356/#review164439 -----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java (line 20) <https://reviews.apache.org/r/56356/#comment236157> In principle, a "star import" is not a good thing, since it makes it more difficult to figure out the full name of some random class Foo being used in the code. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java (line 242) <https://reviews.apache.org/r/56356/#comment236158> Just for a record: if you really care about memory/performance, you can check in advance what this set's size is going to be, and use the HashSet(int size) constructor. The default size is 16, which is sometimes too big and sometimes too small. Hopefully that's not a big issue here. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java (line 254) <https://reviews.apache.org/r/56356/#comment236159> You need 'return' here I guess? ;) sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java (line 262) <https://reviews.apache.org/r/56356/#comment236161> It would be better to move this line to just before the code where this set is populated. You can then allocate it with exact size as well. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java (line 276) <https://reviews.apache.org/r/56356/#comment236160> Here you can just return Collections.emptySet() sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java (line 412) <https://reviews.apache.org/r/56356/#comment236162> Returning the same object is not a good style, because in general, the caller may then want to mutate one object, assuming that the other is different. It's better to always just return Collections.emptySet(). sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 24) <https://reviews.apache.org/r/56356/#comment236163> Same comment about "star import" being not the best thing. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 714) <https://reviews.apache.org/r/56356/#comment236164> Just a note: Google invented things like Sets.newHashSet() etc. in the old days of JDK5/6 I think mainly to avoid long lines like 'Set<MyLongClass> = new HashSet<MyLongClass>();' With the diamond notation in JDK7 there is no such problem anymore, and "native" constructors give you more flexibility, like 'new HashSet<>(exactSize)' etc. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1160) <https://reviews.apache.org/r/56356/#comment236165> Same note: I don't think there is any benefit in using Sets.newXXX(), Lists.newXXX() anymore. - Misha Dmitriev On Feb. 7, 2017, 12:47 a.m., Alexander Kolbasov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56356/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2017, 12:47 a.m.) > > > Review request for sentry, Misha Dmitriev, Hao Hao, kalyan kumar kalvagadda, > Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1615 > https://issues.apache.org/jira/browse/SENTRY-1615 > > > Repository: sentry > > > Description > ------- > > SENTRY-1615 SentryStore should not allocate empty objects that are > immediately returned > > > Diffs > ----- > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > 145808c04c1564eee624f3da2276852e26342c9f > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 321c094366caa2b4a56758781e5fc5a2fc9218d0 > > Diff: https://reviews.apache.org/r/56356/diff/ > > > Testing > ------- > > > Thanks, > > Alexander Kolbasov > >
