You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Brock Noland <br...@cloudera.com> on 2014/01/02 20:50:01 UTC
Review Request 16574: SENTRY-81: Thrift API status should be an object with
status code
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16574/
-----------------------------------------------------------
Review request for sentry.
Bugs: SENTRY-81
https://issues.apache.org/jira/browse/SENTRY-81
Repository: sentry
Description
-------
Since exceptions are crappy in thrift in general and can be problematic from an API perspective as opposed to the thrift API throwing an exception we should return a Status object which has an enum or values and an option message which can be used for errors.
Diffs
-----
sentry-provider/sentry-provider-db/pom.xml 38624c1
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryPolicyStore.java 2d49e2d
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HiveMetaStoreSentryPolicyStoreHandler.java 578b218
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreHandler.java ab50580
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/Status.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/sentry_policystore.thrift 73a5d32
Diff: https://reviews.apache.org/r/16574/diff/
Testing
-------
Thanks,
Brock Noland
Re: Review Request 16574: SENTRY-81: Thrift API status should be an object
with status code
Posted by Shreepadma Venugopalan <sh...@cloudera.com>.
> On Jan. 2, 2014, 7:58 p.m., Shreepadma Venugopalan wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/Status.java, line 47
> > <https://reviews.apache.org/r/16574/diff/1/?file=413464#file413464line47>
> >
> > message and stack seem a little redundant. Should we set message only if stack isn't available?
>
> Brock Noland wrote:
> Right, I think that message and stack would be set together. I just think it's important to differentiate them in case some clients want to use the "message" in their error messages but do not want to parse the stack trace to find the message...
>
> Shreepadma Venugopalan wrote:
> Sounds good. Do you think we should make message required? We should avoid the situation where only error status is returned.
>
> Brock Noland wrote:
> I suppose that would work, we'd just set it to "" when the operation was successful.
Yup, that should work.
- Shreepadma
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16574/#review31054
-----------------------------------------------------------
On Jan. 2, 2014, 7:49 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16574/
> -----------------------------------------------------------
>
> (Updated Jan. 2, 2014, 7:49 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-81
> https://issues.apache.org/jira/browse/SENTRY-81
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Since exceptions are crappy in thrift in general and can be problematic from an API perspective as opposed to the thrift API throwing an exception we should return a Status object which has an enum or values and an option message which can be used for errors.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/pom.xml 38624c1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryPolicyStore.java 2d49e2d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HiveMetaStoreSentryPolicyStoreHandler.java 578b218
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreHandler.java ab50580
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/Status.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/sentry_policystore.thrift 73a5d32
>
> Diff: https://reviews.apache.org/r/16574/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Brock Noland
>
>
Re: Review Request 16574: SENTRY-81: Thrift API status should be an object
with status code
Posted by Brock Noland <br...@cloudera.com>.
> On Jan. 2, 2014, 7:58 p.m., Shreepadma Venugopalan wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/Status.java, line 47
> > <https://reviews.apache.org/r/16574/diff/1/?file=413464#file413464line47>
> >
> > message and stack seem a little redundant. Should we set message only if stack isn't available?
Right, I think that message and stack would be set together. I just think it's important to differentiate them in case some clients want to use the "message" in their error messages but do not want to parse the stack trace to find the message...
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16574/#review31054
-----------------------------------------------------------
On Jan. 2, 2014, 7:49 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16574/
> -----------------------------------------------------------
>
> (Updated Jan. 2, 2014, 7:49 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-81
> https://issues.apache.org/jira/browse/SENTRY-81
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Since exceptions are crappy in thrift in general and can be problematic from an API perspective as opposed to the thrift API throwing an exception we should return a Status object which has an enum or values and an option message which can be used for errors.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/pom.xml 38624c1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryPolicyStore.java 2d49e2d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HiveMetaStoreSentryPolicyStoreHandler.java 578b218
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreHandler.java ab50580
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/Status.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/sentry_policystore.thrift 73a5d32
>
> Diff: https://reviews.apache.org/r/16574/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Brock Noland
>
>
Re: Review Request 16574: SENTRY-81: Thrift API status should be an object
with status code
Posted by Shreepadma Venugopalan <sh...@cloudera.com>.
> On Jan. 2, 2014, 7:58 p.m., Shreepadma Venugopalan wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/Status.java, line 47
> > <https://reviews.apache.org/r/16574/diff/1/?file=413464#file413464line47>
> >
> > message and stack seem a little redundant. Should we set message only if stack isn't available?
>
> Brock Noland wrote:
> Right, I think that message and stack would be set together. I just think it's important to differentiate them in case some clients want to use the "message" in their error messages but do not want to parse the stack trace to find the message...
Sounds good. Do you think we should make message required? We should avoid the situation where only error status is returned.
- Shreepadma
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16574/#review31054
-----------------------------------------------------------
On Jan. 2, 2014, 7:49 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16574/
> -----------------------------------------------------------
>
> (Updated Jan. 2, 2014, 7:49 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-81
> https://issues.apache.org/jira/browse/SENTRY-81
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Since exceptions are crappy in thrift in general and can be problematic from an API perspective as opposed to the thrift API throwing an exception we should return a Status object which has an enum or values and an option message which can be used for errors.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/pom.xml 38624c1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryPolicyStore.java 2d49e2d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HiveMetaStoreSentryPolicyStoreHandler.java 578b218
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreHandler.java ab50580
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/Status.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/sentry_policystore.thrift 73a5d32
>
> Diff: https://reviews.apache.org/r/16574/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Brock Noland
>
>
Re: Review Request 16574: SENTRY-81: Thrift API status should be an object
with status code
Posted by Brock Noland <br...@cloudera.com>.
> On Jan. 2, 2014, 7:58 p.m., Shreepadma Venugopalan wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/Status.java, line 47
> > <https://reviews.apache.org/r/16574/diff/1/?file=413464#file413464line47>
> >
> > message and stack seem a little redundant. Should we set message only if stack isn't available?
>
> Brock Noland wrote:
> Right, I think that message and stack would be set together. I just think it's important to differentiate them in case some clients want to use the "message" in their error messages but do not want to parse the stack trace to find the message...
>
> Shreepadma Venugopalan wrote:
> Sounds good. Do you think we should make message required? We should avoid the situation where only error status is returned.
I suppose that would work, we'd just set it to "" when the operation was successful.
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16574/#review31054
-----------------------------------------------------------
On Jan. 2, 2014, 7:49 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16574/
> -----------------------------------------------------------
>
> (Updated Jan. 2, 2014, 7:49 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-81
> https://issues.apache.org/jira/browse/SENTRY-81
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Since exceptions are crappy in thrift in general and can be problematic from an API perspective as opposed to the thrift API throwing an exception we should return a Status object which has an enum or values and an option message which can be used for errors.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/pom.xml 38624c1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryPolicyStore.java 2d49e2d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HiveMetaStoreSentryPolicyStoreHandler.java 578b218
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreHandler.java ab50580
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/Status.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/sentry_policystore.thrift 73a5d32
>
> Diff: https://reviews.apache.org/r/16574/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Brock Noland
>
>
Re: Review Request 16574: SENTRY-81: Thrift API status should be an object
with status code
Posted by Shreepadma Venugopalan <sh...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16574/#review31054
-----------------------------------------------------------
LGTM mostly. Please see the comment below.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/Status.java
<https://reviews.apache.org/r/16574/#comment59396>
message and stack seem a little redundant. Should we set message only if stack isn't available?
- Shreepadma Venugopalan
On Jan. 2, 2014, 7:49 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16574/
> -----------------------------------------------------------
>
> (Updated Jan. 2, 2014, 7:49 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-81
> https://issues.apache.org/jira/browse/SENTRY-81
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Since exceptions are crappy in thrift in general and can be problematic from an API perspective as opposed to the thrift API throwing an exception we should return a Status object which has an enum or values and an option message which can be used for errors.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/pom.xml 38624c1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryPolicyStore.java 2d49e2d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HiveMetaStoreSentryPolicyStoreHandler.java 578b218
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreHandler.java ab50580
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/Status.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/sentry_policystore.thrift 73a5d32
>
> Diff: https://reviews.apache.org/r/16574/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Brock Noland
>
>
Re: Review Request 16574: SENTRY-81: Thrift API status should be an object
with status code
Posted by Shreepadma Venugopalan <sh...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16574/#review31061
-----------------------------------------------------------
Ship it!
Ship It!
- Shreepadma Venugopalan
On Jan. 2, 2014, 8:11 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16574/
> -----------------------------------------------------------
>
> (Updated Jan. 2, 2014, 8:11 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-81
> https://issues.apache.org/jira/browse/SENTRY-81
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Since exceptions are crappy in thrift in general and can be problematic from an API perspective as opposed to the thrift API throwing an exception we should return a Status object which has an enum or values and an option message which can be used for errors.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/pom.xml 38624c1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryPolicyStore.java 2d49e2d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HiveMetaStoreSentryPolicyStoreHandler.java 578b218
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreHandler.java ab50580
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/Status.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/sentry_policystore.thrift 73a5d32
>
> Diff: https://reviews.apache.org/r/16574/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Brock Noland
>
>
Re: Review Request 16574: SENTRY-81: Thrift API status should be an object
with status code
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16574/
-----------------------------------------------------------
(Updated Jan. 2, 2014, 8:11 p.m.)
Review request for sentry.
Changes
-------
Addressed feedback.
Bugs: SENTRY-81
https://issues.apache.org/jira/browse/SENTRY-81
Repository: sentry
Description
-------
Since exceptions are crappy in thrift in general and can be problematic from an API perspective as opposed to the thrift API throwing an exception we should return a Status object which has an enum or values and an option message which can be used for errors.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/pom.xml 38624c1
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryPolicyStore.java 2d49e2d
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HiveMetaStoreSentryPolicyStoreHandler.java 578b218
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreHandler.java ab50580
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/Status.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/sentry_policystore.thrift 73a5d32
Diff: https://reviews.apache.org/r/16574/diff/
Testing
-------
Thanks,
Brock Noland