----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62514/#review186418 -----------------------------------------------------------
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java Lines 33 (patched) <https://reviews.apache.org/r/62514/#comment262977> I don't think KeyValue should accept intern/lowercase parameters. KeyValue only stores key-value pairs, and the lowercase and intern should be done out of the contructor. I don't know about intern yet, but at least lowercase can be applied to the keyValue parameter before calling this constructor. Regarding intern, is there another way to intern this object? sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java Line 55 (original), 55 (patched) <https://reviews.apache.org/r/62514/#comment262978> Javadoc mentions that ImmutableList.copyOf() will avoid to copy the values but it does not guarantee that and it may be subject to change. We could easly avoid this change if we use the ImmutableList.Builder() and add elements to it instead of using a List() and then calling ImmutableList.copyOf() that "could" copy values. See https://google.github.io/guava/releases/19.0/api/docs/com/google/common/collect/ImmutableList.Builder.html sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java Lines 54-56 (original), 56-58 (patched) <https://reviews.apache.org/r/62514/#comment262979> This 'Privilege privilege = ' is only created to get a list of KeyValue pairs without the PRIVILEGE_NAME. It seems unecessary as CommonPrivilege makes two copies of the KeyValue list (one in the constructor and another in the getAuthorizable() method). Should it better to create a static method that returns this KeyValue list instead? Why are you doing a copy to immutable list? Java tries to avoid making copies but it does not guarantee this would happen in the future. We should find a better way to keep immutable values when they are created instead of calling copyof many times. sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java Line 76 (original), 81 (patched) <https://reviews.apache.org/r/62514/#comment262980> This is also making another copy of KeyValue pairs only to filter the URI from the list. Is there a way to avoid this? sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java Line 83 (original), 88 (patched) <https://reviews.apache.org/r/62514/#comment262981> his is only walking through each item of the KeyValue pairs and store a cache of the values. Could this list be replaced by interned strings instead? Why do we keep a cache here btw? - Sergio Pena On Sept. 22, 2017, 6:43 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62514/ > ----------------------------------------------------------- > > (Updated Sept. 22, 2017, 6:43 p.m.) > > > Review request for sentry, Alexander Kolbasov, Brian Towles, Kalyani > Kashikar, and Sergio Pena. > > > Repository: sentry > > > Description > ------- > > reduce memory usage by represent the matching key as list of KeyValue and > intern hte value > > > Diffs > ----- > > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java > 4e944e5 > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java > ab55609 > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java > 0ad4616 > > sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java > 891c1d9 > > > Diff: https://reviews.apache.org/r/62514/diff/1/ > > > Testing > ------- > > unit tests > > > Thanks, > > Na Li > >
