Re: Review Request 56356: SENTRY-1615 SentryStore should not allocate empty objects that are immediately returned

2017-02-08 Thread kalyan kumar kalvagadda 

---
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)


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)


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)


Ditto



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 (line 2196)


ditto



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 (line 2277)


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

2017-02-07 Thread Vadim Spector 

---
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

2017-02-07 Thread Vamsee Yarlagadda 

---
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

2017-02-06 Thread Misha Dmitriev 

---
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

2017-02-06 Thread Alexander Kolbasov 


> 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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 = new HashSet();' 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

2017-02-06 Thread Misha Dmitriev 

---
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)


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)


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)


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)


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)


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)


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)


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)


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 = new 
HashSet();' 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)


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

2017-02-06 Thread Alexander Kolbasov 

---
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

Review Request 56356: SENTRY-1615 SentryStore should not allocate empty objects that are immediately returned

2017-02-06 Thread Alexander Kolbasov 

---
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