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/01/17 00:26:24 UTC
Review Request 55594: SENTRY-1594 TransactionBlock should become
generic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55594/
-----------------------------------------------------------
Review request for sentry, Colin Ma, Misha Dmitriev, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
Bugs: SENTRY-1594
https://issues.apache.org/jira/browse/SENTRY-1594
Repository: sentry
Description
-------
SENTRY-1594 TransactionBlock should become generic
Diffs
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 57b6effa1b1bbfa22fbc2114db1e502365343207
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java b7ae63430cddb81cb3235a7b04ecf95410d8604f
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionBlock.java 7a57a9658b1b0e6b36aec2c61d187527b0adee4e
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java 85a5326ba52fc532f3d933f7e62a352f4d4080df
Diff: https://reviews.apache.org/r/55594/diff/
Testing
-------
Thanks,
Alexander Kolbasov
Re: Review Request 55594: SENTRY-1594 TransactionBlock should become
generic
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55594/#review161905
-----------------------------------------------------------
Ship it!
Ship It!
- Hao Hao
On Jan. 17, 2017, 12:26 a.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55594/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2017, 12:26 a.m.)
>
>
> Review request for sentry, Colin Ma, Misha Dmitriev, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1594
> https://issues.apache.org/jira/browse/SENTRY-1594
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1594 TransactionBlock should become generic
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 57b6effa1b1bbfa22fbc2114db1e502365343207
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java b7ae63430cddb81cb3235a7b04ecf95410d8604f
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionBlock.java 7a57a9658b1b0e6b36aec2c61d187527b0adee4e
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java 85a5326ba52fc532f3d933f7e62a352f4d4080df
>
> Diff: https://reviews.apache.org/r/55594/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 55594: SENTRY-1594 TransactionBlock should become
generic
Posted by Misha Dmitriev <mi...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55594/#review161908
-----------------------------------------------------------
Fix it, then Ship it!
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java (line 132)
<https://reviews.apache.org/r/55594/#comment233188>
Nit: if this is a one-line comment in the code (rather than javadoc), I don't see why it should take 3 lines. Can be just "// check with grant option".
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java (line 155)
<https://reviews.apache.org/r/55594/#comment233189>
Same as above.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java (line 215)
<https://reviews.apache.org/r/55594/#comment233190>
Nit: space after comma.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java (line 385)
<https://reviews.apache.org/r/55594/#comment233191>
Same as above - why waste a couple of lines here.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 986)
<https://reviews.apache.org/r/55594/#comment233193>
It's a good practice to avoid creating a new object every time we want to return an empty result. Use Collections.EMPTY_LIST instead.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1035)
<https://reviews.apache.org/r/55594/#comment233194>
Nit: why put this on a separate line?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1043)
<https://reviews.apache.org/r/55594/#comment233195>
Nit: why spread this over so many lines?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1081)
<https://reviews.apache.org/r/55594/#comment233197>
Nit: why add this 'result' variable, why not return query.execute... right away?
- Misha Dmitriev
On Jan. 17, 2017, 12:26 a.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55594/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2017, 12:26 a.m.)
>
>
> Review request for sentry, Colin Ma, Misha Dmitriev, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1594
> https://issues.apache.org/jira/browse/SENTRY-1594
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1594 TransactionBlock should become generic
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 57b6effa1b1bbfa22fbc2114db1e502365343207
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java b7ae63430cddb81cb3235a7b04ecf95410d8604f
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionBlock.java 7a57a9658b1b0e6b36aec2c61d187527b0adee4e
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java 85a5326ba52fc532f3d933f7e62a352f4d4080df
>
> Diff: https://reviews.apache.org/r/55594/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>