Re: Review Request 29039: SENTRY-567: Thrift changes

[Lenni Kuff <ls...@cloudera.com>]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29039/#review66925
-----------------------------------------------------------



sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
<https://reviews.apache.org/r/29039/#comment110635>

    Let's be sure to comment on all of the Thrift structs to specify what they are used for. They are part of the interface to the service.
    
    Currently it's a bit confusing the similarly named StoreAuthorizeable and SentryAuthorizeable.



sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
<https://reviews.apache.org/r/29039/#comment110647>

    SentryStoreOpType



sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
<https://reviews.apache.org/r/29039/#comment110636>

    Should NO_OP be 0?



sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
<https://reviews.apache.org/r/29039/#comment110637>

    Comment on what is expected here.



sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
<https://reviews.apache.org/r/29039/#comment110639>

    Explain what "name" is.



sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
<https://reviews.apache.org/r/29039/#comment110638>

    more descriptive field name than "type"?



sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
<https://reviews.apache.org/r/29039/#comment110642>

    Explain under what conditions optional fields will not be set.



sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
<https://reviews.apache.org/r/29039/#comment110641>

    The name makes it sound like this is a full snapshot of the Sentry store.



sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
<https://reviews.apache.org/r/29039/#comment110640>

    explain what the expected key is for each for the maps.



sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
<https://reviews.apache.org/r/29039/#comment110646>

    Maybe just TStoreRecord ? Sentry is kind of implied.



sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
<https://reviews.apache.org/r/29039/#comment110645>

    What is the newAuthorizeable field? Is that when the user performs a rename? Do you think we should seperate the "operation" aspects from the "record" using a second TSentryStoreOp struct that contains a TRecord field?



sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
<https://reviews.apache.org/r/29039/#comment110643>

    Should the version be a string of a long? What is the version comment?


- Lenni Kuff


On Dec. 15, 2014, 9:03 a.m., Arun Suresh wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29039/
> -----------------------------------------------------------
> 
> (Updated Dec. 15, 2014, 9:03 a.m.)
> 
> 
> Review request for sentry, bc Wong, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> All thrift Changes related to the JIRA
> More decription on the JIRA : issues.apache.org/jira/browse/SENTRY-567
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift 993ea46 
> 
> Diff: https://reviews.apache.org/r/29039/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Arun Suresh
> 
>