----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59252/#review174876 -----------------------------------------------------------
The comments that I made in the first few occurrences of the potential issues apply everywhere else where you use String.intern() or rewrite hashCode(). sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java Line 30 (original), 30 (patched) <https://reviews.apache.org/r/59252/#comment248100> Looks like path can be null, in which case you will get NPE here. When I worked on similar code in Hive, I ended up with a special static method String internIfNotNull(String s) in some common util class, to avoid writing the same line over and over again. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java Line 41 (original), 41 (patched) <https://reviews.apache.org/r/59252/#comment248101> I don't know whether 31 is a more sensible hashcode than 0 when path is null... maybe it just makes sense to keep this compatible with the old code? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java Line 82 (original), 82 (patched) <https://reviews.apache.org/r/59252/#comment248102> Probably it's safer to use the proposed internIfNotNull() everywhere to avoid any problems. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGroup.java Line 86 (original), 82 (patched) <https://reviews.apache.org/r/59252/#comment248103> Similar consideration about 0 vs 31. - Misha Dmitriev On May 13, 2017, 12:54 a.m., Alexander Kolbasov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59252/ > ----------------------------------------------------------- > > (Updated May 13, 2017, 12:54 a.m.) > > > Review request for sentry, Brian Towles, Misha Dmitriev, Hao Hao, kalyan > kumar kalvagadda, Na Li, and Vamsee Yarlagadda. > > > Bugs: SENTRY-1758 > https://issues.apache.org/jira/browse/SENTRY-1758 > > > Repository: sentry > > > Description > ------- > > SENTRY-1758 Improve Sentry memory usage by interning object names > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java > c710701e06921c5ee85cda5c11021384aeb4053f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java > c74384688ca920c79fb2987d225042e135cdfd53 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java > 749af2a3e9d0420d3597952b0fcf13578f16a52f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGroup.java > 7e41c93294c64dd50b5672ca6fa3e003955ed365 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java > 4c3af7992c90ba6ce33ff38ca6c5a3eb3492dd8b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > f9da589d8de78bd4f8cb820bd7f545241000c417 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUser.java > ff5724999531fe4c1c6ccf8e206ee695749661c4 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryVersion.java > ff8830f11a4f53801babbc0bb5deb386cfcbab42 > > > Diff: https://reviews.apache.org/r/59252/diff/1/ > > > Testing > ------- > > > Thanks, > > Alexander Kolbasov > >
