You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Alexander Kolbasov <ak...@gmail.com> on 2017/02/07 00:41:14 UTC
Review Request 56356: SENTRY-1615 SentryStore should not allocate
empty objects that are immediately returned
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56356/
-----------------------------------------------------------
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
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegesAtFunctionScope.java cebad987c89c7950e28c700c3fe398d409280163
Diff: https://reviews.apache.org/r/56356/diff/
Testing
-------
Thanks,
Alexander Kolbasov
Re: Review Request 56356: SENTRY-1615 SentryStore should not allocate
empty objects that are immediately returned
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56356/#review164560
-----------------------------------------------------------
Ship it!
Ship It!
- Vamsee Yarlagadda
On Feb. 7, 2017, 2:14 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, 2:14 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
>
>
Re: Review Request 56356: SENTRY-1615 SentryStore should not allocate
empty objects that are immediately returned
Posted by Misha Dmitriev <mi...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56356/#review164457
-----------------------------------------------------------
Ship it!
Ship It!
- Misha Dmitriev
On Feb. 7, 2017, 2:14 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, 2:14 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
>
>
Re: Review Request 56356: SENTRY-1615 SentryStore should not allocate
empty objects that are immediately returned
Posted by Vadim Spector <vs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56356/#review164566
-----------------------------------------------------------
Ship it!
Ship It!
- Vadim Spector
On Feb. 7, 2017, 2:14 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, 2:14 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
>
>
Re: Review Request 56356: SENTRY-1615 SentryStore should not allocate
empty objects that are immediately returned
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On Feb. 8, 2017, 2:35 p.m., kalyan kumar kalvagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java, line 286
> > <https://reviews.apache.org/r/56356/diff/4/?file=1625886#file1625886line286>
> >
> > Are we sure that query.execute does not return null?
Why do you think that it can return null for a collection? It can return null when it returns a single object - e.g. when setUnique is true. Can you point out some specific documentation or code that shows that this is possible?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56356/#review164562
-----------------------------------------------------------
On Feb. 7, 2017, 2:14 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, 2:14 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
>
>
Re: Review Request 56356: SENTRY-1615 SentryStore should not allocate
empty objects that are immediately returned
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56356/#review164562
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java (line 280)
<https://reviews.apache.org/r/56356/#comment236331>
Are we sure that query.execute does not return null?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 2117)
<https://reviews.apache.org/r/56356/#comment236451>
mSentryRoles here is returned by qury.execute(). As per datanucleus documentation it can return null.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 2139)
<https://reviews.apache.org/r/56356/#comment236452>
Ditto
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 2196)
<https://reviews.apache.org/r/56356/#comment236453>
ditto
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 2277)
<https://reviews.apache.org/r/56356/#comment236454>
query.execute can return null.
- kalyan kumar kalvagadda
On Feb. 7, 2017, 2:14 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, 2:14 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
>
>
Re: Review Request 56356: SENTRY-1615 SentryStore should not allocate
empty objects that are immediately returned
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56356/
-----------------------------------------------------------
(Updated Feb. 7, 2017, 2:14 a.m.)
Review request for sentry, Misha Dmitriev, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Addressed code review comments
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 (updated)
-----
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
Re: Review Request 56356: SENTRY-1615 SentryStore should not allocate
empty objects that are immediately returned
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On Feb. 7, 2017, 1:01 a.m., Misha Dmitriev wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java, line 247
> > <https://reviews.apache.org/r/56356/diff/2/?file=1625810#file1625810line247>
> >
> > 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.
Thanks for the comment. I don't think it matters much here.
> On Feb. 7, 2017, 1:01 a.m., Misha Dmitriev wrote:
> > 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/diff/2/?file=1625810#file1625810line20>
> >
> > 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.
Fixed.
> On Feb. 7, 2017, 1:01 a.m., Misha Dmitriev wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 728
> > <https://reviews.apache.org/r/56356/diff/2/?file=1625811#file1625811line728>
> >
> > 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.
The reason I changed it was for consistency - I can change it all to diamond notation instead.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56356/#review164439
-----------------------------------------------------------
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
>
>
Re: Review Request 56356: SENTRY-1615 SentryStore should not allocate
empty objects that are immediately returned
Posted by Misha Dmitriev <mi...@cloudera.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 56356: SENTRY-1615 SentryStore should not allocate
empty objects that are immediately returned
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Fixed typo with missing return statement
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 (updated)
-----
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
Re: Review Request 56356: SENTRY-1615 SentryStore should not allocate
empty objects that are immediately returned
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56356/
-----------------------------------------------------------
(Updated Feb. 7, 2017, 12:44 a.m.)
Review request for sentry, Misha Dmitriev, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Removed stray change in the test file
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 (updated)
-----
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