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 2016/12/03 06:52:52 UTC
Review Request 54338: SENTRY-1515: Cleanup exception handling in
SentryStore
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/
-----------------------------------------------------------
Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
Repository: sentry
Description
-------
SENTRY-1515: Cleanup exception handling in SentryStore
Diffs
-----
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 3695709e03e683afe6196def53883e37e4910a1c
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java 73872813fb37428529f674fef924f5a05d23c2f6
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java 3d756c914a1d79db15ab66eaa657c96d70e0dd1c
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 2ee06f9f236694f87beb9466285bb6363a0007de
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java f717f38cb14ea7594f87ec6c6bf30b78241dfed6
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9
Diff: https://reviews.apache.org/r/54338/diff/
Testing
-------
Thanks,
Alexander Kolbasov
Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in
SentryStore
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On Dec. 5, 2016, 9:25 p.m., Vadim Spector wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java, line 79
> > <https://reviews.apache.org/r/54338/diff/1/?file=1575128#file1575128line79>
> >
> > do we still need ctors to be public, if we have public static create()?
It is used by UpdateForwarderWithHA, so it can't be private, but it can be package-private.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/#review158055
-----------------------------------------------------------
On Dec. 3, 2016, 6:52 a.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54338/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2016, 6:52 a.m.)
>
>
> Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1515: Cleanup exception handling in SentryStore
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 3695709e03e683afe6196def53883e37e4910a1c
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java 73872813fb37428529f674fef924f5a05d23c2f6
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java 3d756c914a1d79db15ab66eaa657c96d70e0dd1c
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 2ee06f9f236694f87beb9466285bb6363a0007de
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java f717f38cb14ea7594f87ec6c6bf30b78241dfed6
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9
>
> Diff: https://reviews.apache.org/r/54338/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in
SentryStore
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On Dec. 5, 2016, 9:25 p.m., Vadim Spector wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java, line 192
> > <https://reviews.apache.org/r/54338/diff/1/?file=1575128#file1575128line192>
> >
> > a) why isn't it an error?
> > b) LOGGER.warn() would print stack trace anyway - do we need e.printStackTrace()?
I was looking at implemantation of LOGGER in www.slf4j.org. It looks like warn implementation calls printStackTrace on the throwable object.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/#review158055
-----------------------------------------------------------
On Dec. 3, 2016, 6:52 a.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54338/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2016, 6:52 a.m.)
>
>
> Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1515: Cleanup exception handling in SentryStore
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 3695709e03e683afe6196def53883e37e4910a1c
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java 73872813fb37428529f674fef924f5a05d23c2f6
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java 3d756c914a1d79db15ab66eaa657c96d70e0dd1c
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 2ee06f9f236694f87beb9466285bb6363a0007de
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java f717f38cb14ea7594f87ec6c6bf30b78241dfed6
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9
>
> Diff: https://reviews.apache.org/r/54338/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in
SentryStore
Posted by Vadim Spector <vs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/#review158055
-----------------------------------------------------------
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java (line 130)
<https://reviews.apache.org/r/54338/#comment228727>
add try-finally for timerContext to behave correctly?
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java (line 79)
<https://reviews.apache.org/r/54338/#comment228728>
do we still need ctors to be public, if we have public static create()?
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java (line 192)
<https://reviews.apache.org/r/54338/#comment228730>
a) why isn't it an error?
b) LOGGER.warn() would print stack trace anyway - do we need e.printStackTrace()?
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java (line 202)
<https://reviews.apache.org/r/54338/#comment228731>
a) why isn't it an error?
b) LOGGER.warn() would print stack trace anyway - do we need e.printStackTrace()?
- Vadim Spector
On Dec. 3, 2016, 6:52 a.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54338/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2016, 6:52 a.m.)
>
>
> Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1515: Cleanup exception handling in SentryStore
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 3695709e03e683afe6196def53883e37e4910a1c
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java 73872813fb37428529f674fef924f5a05d23c2f6
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java 3d756c914a1d79db15ab66eaa657c96d70e0dd1c
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 2ee06f9f236694f87beb9466285bb6363a0007de
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java f717f38cb14ea7594f87ec6c6bf30b78241dfed6
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9
>
> Diff: https://reviews.apache.org/r/54338/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in
SentryStore
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/#review159242
-----------------------------------------------------------
Ship it!
Ship It!
- Vamsee Yarlagadda
On Dec. 7, 2016, 11:42 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54338/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2016, 11:42 p.m.)
>
>
> Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1515: Cleanup exception handling in SentryStore
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 3695709e03e683afe6196def53883e37e4910a1c
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java 73872813fb37428529f674fef924f5a05d23c2f6
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java 3d756c914a1d79db15ab66eaa657c96d70e0dd1c
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 2ee06f9f236694f87beb9466285bb6363a0007de
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java f717f38cb14ea7594f87ec6c6bf30b78241dfed6
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9
>
> Diff: https://reviews.apache.org/r/54338/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in
SentryStore
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On Dec. 14, 2016, 10:50 p.m., Vadim Spector wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java, line 154
> > <https://reviews.apache.org/r/54338/diff/2/?file=1579347#file1579347line154>
> >
> > Timer.Context.stop() per javadoc is equivalent to calling Timer.Context.close() as in AutoCloseable, so it seems like with converting to try-with-resource we call it twice. May want to look for other similar cases too.
Good point, will fix. I think this is the only place where we use try-with-resource to close timers.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/#review159245
-----------------------------------------------------------
On Dec. 7, 2016, 11:42 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54338/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2016, 11:42 p.m.)
>
>
> Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1515: Cleanup exception handling in SentryStore
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 3695709e03e683afe6196def53883e37e4910a1c
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java 73872813fb37428529f674fef924f5a05d23c2f6
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java 3d756c914a1d79db15ab66eaa657c96d70e0dd1c
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 2ee06f9f236694f87beb9466285bb6363a0007de
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java f717f38cb14ea7594f87ec6c6bf30b78241dfed6
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9
>
> Diff: https://reviews.apache.org/r/54338/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in
SentryStore
Posted by Vadim Spector <vs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/#review159245
-----------------------------------------------------------
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java (line 154)
<https://reviews.apache.org/r/54338/#comment230190>
Timer.Context.stop() per javadoc is equivalent to calling Timer.Context.close() as in AutoCloseable, so it seems like with converting to try-with-resource we call it twice. May want to look for other similar cases too.
- Vadim Spector
On Dec. 7, 2016, 11:42 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54338/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2016, 11:42 p.m.)
>
>
> Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1515: Cleanup exception handling in SentryStore
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 3695709e03e683afe6196def53883e37e4910a1c
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java 73872813fb37428529f674fef924f5a05d23c2f6
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java 3d756c914a1d79db15ab66eaa657c96d70e0dd1c
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 2ee06f9f236694f87beb9466285bb6363a0007de
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java f717f38cb14ea7594f87ec6c6bf30b78241dfed6
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9
>
> Diff: https://reviews.apache.org/r/54338/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in
SentryStore
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/#review160548
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 2238)
<https://reviews.apache.org/r/54338/#comment231696>
How would you suggest to handle these errors inside these functions?
This changeset is restoring the status quo as it existed before SENTRY-1422 and just makes exceptions more explicit.
- Alexander Kolbasov
On Dec. 14, 2016, 11:57 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54338/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2016, 11:57 p.m.)
>
>
> Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1515: Cleanup exception handling in SentryStore
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 3695709e03e683afe6196def53883e37e4910a1c
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java 73872813fb37428529f674fef924f5a05d23c2f6
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java 3d756c914a1d79db15ab66eaa657c96d70e0dd1c
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 2ee06f9f236694f87beb9466285bb6363a0007de
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java f717f38cb14ea7594f87ec6c6bf30b78241dfed6
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java e8d0e2505c5b178cae3d27e7d85caa652f630a2d
>
> Diff: https://reviews.apache.org/r/54338/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in
SentryStore
Posted by Mat Crocker <ma...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/#review160923
-----------------------------------------------------------
Ship it!
looks like basic try/catch clean up, looks good to me.
- Mat Crocker
On Jan. 5, 2017, 8:28 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54338/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2017, 8:28 p.m.)
>
>
> Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1515
> https://issues.apache.org/jira/browse/SENTRY-1515
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1515: Cleanup exception handling in SentryStore
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 3695709e03e683afe6196def53883e37e4910a1c
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java 73872813fb37428529f674fef924f5a05d23c2f6
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java 3d756c914a1d79db15ab66eaa657c96d70e0dd1c
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 2ee06f9f236694f87beb9466285bb6363a0007de
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java f717f38cb14ea7594f87ec6c6bf30b78241dfed6
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java e8d0e2505c5b178cae3d27e7d85caa652f630a2d
>
> Diff: https://reviews.apache.org/r/54338/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in
SentryStore
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/#review160952
-----------------------------------------------------------
Ship it!
- Hao Hao
On Jan. 5, 2017, 8:28 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54338/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2017, 8:28 p.m.)
>
>
> Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1515
> https://issues.apache.org/jira/browse/SENTRY-1515
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1515: Cleanup exception handling in SentryStore
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 3695709e03e683afe6196def53883e37e4910a1c
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java 73872813fb37428529f674fef924f5a05d23c2f6
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java 3d756c914a1d79db15ab66eaa657c96d70e0dd1c
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 2ee06f9f236694f87beb9466285bb6363a0007de
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java f717f38cb14ea7594f87ec6c6bf30b78241dfed6
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java e8d0e2505c5b178cae3d27e7d85caa652f630a2d
>
> Diff: https://reviews.apache.org/r/54338/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in
SentryStore
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/
-----------------------------------------------------------
(Updated Jan. 5, 2017, 8:28 p.m.)
Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
Bugs: SENTRY-1515
https://issues.apache.org/jira/browse/SENTRY-1515
Repository: sentry
Description
-------
SENTRY-1515: Cleanup exception handling in SentryStore
Diffs
-----
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 3695709e03e683afe6196def53883e37e4910a1c
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java 73872813fb37428529f674fef924f5a05d23c2f6
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java 3d756c914a1d79db15ab66eaa657c96d70e0dd1c
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 2ee06f9f236694f87beb9466285bb6363a0007de
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java f717f38cb14ea7594f87ec6c6bf30b78241dfed6
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java e8d0e2505c5b178cae3d27e7d85caa652f630a2d
Diff: https://reviews.apache.org/r/54338/diff/
Testing
-------
Thanks,
Alexander Kolbasov
Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in
SentryStore
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/#review160008
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 2238)
<https://reviews.apache.org/r/54338/#comment231018>
What is the advantage of throwing the exceptions, instead of handlinging them in side these functions.
- kalyan kumar kalvagadda
On Dec. 14, 2016, 11:57 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54338/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2016, 11:57 p.m.)
>
>
> Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1515: Cleanup exception handling in SentryStore
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 3695709e03e683afe6196def53883e37e4910a1c
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java 73872813fb37428529f674fef924f5a05d23c2f6
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java 3d756c914a1d79db15ab66eaa657c96d70e0dd1c
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 2ee06f9f236694f87beb9466285bb6363a0007de
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java f717f38cb14ea7594f87ec6c6bf30b78241dfed6
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java e8d0e2505c5b178cae3d27e7d85caa652f630a2d
>
> Diff: https://reviews.apache.org/r/54338/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in
SentryStore
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/
-----------------------------------------------------------
(Updated Dec. 14, 2016, 11:57 p.m.)
Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Addressed code review comment for timer stop() being called twice
Repository: sentry
Description
-------
SENTRY-1515: Cleanup exception handling in SentryStore
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 3695709e03e683afe6196def53883e37e4910a1c
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java 73872813fb37428529f674fef924f5a05d23c2f6
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java 3d756c914a1d79db15ab66eaa657c96d70e0dd1c
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 2ee06f9f236694f87beb9466285bb6363a0007de
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java f717f38cb14ea7594f87ec6c6bf30b78241dfed6
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java e8d0e2505c5b178cae3d27e7d85caa652f630a2d
Diff: https://reviews.apache.org/r/54338/diff/
Testing
-------
Thanks,
Alexander Kolbasov
Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in
SentryStore
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/
-----------------------------------------------------------
(Updated Dec. 7, 2016, 11:42 p.m.)
Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
Repository: sentry
Description
-------
SENTRY-1515: Cleanup exception handling in SentryStore
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 3695709e03e683afe6196def53883e37e4910a1c
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java 73872813fb37428529f674fef924f5a05d23c2f6
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java 3d756c914a1d79db15ab66eaa657c96d70e0dd1c
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 2ee06f9f236694f87beb9466285bb6363a0007de
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java f717f38cb14ea7594f87ec6c6bf30b78241dfed6
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9
Diff: https://reviews.apache.org/r/54338/diff/
Testing
-------
Thanks,
Alexander Kolbasov
Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in
SentryStore
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On Dec. 5, 2016, 8:24 p.m., Vamsee Yarlagadda wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java, lines 188-194
> > <https://reviews.apache.org/r/54338/diff/1/?file=1575128#file1575128line188>
> >
> > A general question outside of this jira.
> >
> > This still makes me question the fact that when an update notification gets invoked, there is a chance that the update has failed to process properly and all we are doing is to simply log the error and move on. How does the caller get the notification that something didn't go as expected so that they can take some action on it?
Seems like there isn't much we can do anyway. We can exit but it wouldn't help either. ANyway, it is a different issue.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/#review158054
-----------------------------------------------------------
On Dec. 3, 2016, 6:52 a.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54338/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2016, 6:52 a.m.)
>
>
> Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1515: Cleanup exception handling in SentryStore
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 3695709e03e683afe6196def53883e37e4910a1c
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java 73872813fb37428529f674fef924f5a05d23c2f6
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java 3d756c914a1d79db15ab66eaa657c96d70e0dd1c
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 2ee06f9f236694f87beb9466285bb6363a0007de
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java f717f38cb14ea7594f87ec6c6bf30b78241dfed6
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9
>
> Diff: https://reviews.apache.org/r/54338/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in
SentryStore
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54338/#review158054
-----------------------------------------------------------
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java (lines 188 - 194)
<https://reviews.apache.org/r/54338/#comment228726>
A general question outside of this jira.
This still makes me question the fact that when an update notification gets invoked, there is a chance that the update has failed to process properly and all we are doing is to simply log the error and move on. How does the caller get the notification that something didn't go as expected so that they can take some action on it?
- Vamsee Yarlagadda
On Dec. 3, 2016, 6:52 a.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54338/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2016, 6:52 a.m.)
>
>
> Review request for sentry, Colin Ma, Hao Hao, kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1515: Cleanup exception handling in SentryStore
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 3695709e03e683afe6196def53883e37e4910a1c
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java 73872813fb37428529f674fef924f5a05d23c2f6
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java 3d756c914a1d79db15ab66eaa657c96d70e0dd1c
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 2ee06f9f236694f87beb9466285bb6363a0007de
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java f717f38cb14ea7594f87ec6c6bf30b78241dfed6
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9
>
> Diff: https://reviews.apache.org/r/54338/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>