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