----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27161/#review58646 -----------------------------------------------------------
Overall the patch looks fine to me. I do have a bit of concern about the way we are handling the default value in the RPC request. If the Sentry server doesn't have the config property set, then it will return the default passed by the caller. This would make it impossible for the client to determine if the service indeed has that property set. Note that when the server is executing the code that looks at this config property, it could have a different default than the one passed by client is getConfig(). Hence the client can't realiably assume the config value returned by the RPC. Should we make the default as optional ? If the default is not set and server doesn't have the property configured, then it can return an error back ? - Prasad Mujumdar On Oct. 27, 2014, 6:17 p.m., Mike Yoder wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27161/ > ----------------------------------------------------------- > > (Updated Oct. 27, 2014, 6:17 p.m.) > > > Review request for sentry and Lenni Kuff. > > > Bugs: SENTRY-415 > https://issues.apache.org/jira/browse/SENTRY-415 > > > Repository: sentry > > > Description > ------- > > Add API to Sentry Service that allows clients to read the service's config > values. > > A new RPC was added to sentry_policy_service.thrift, thrift code rebuilt, and > new files added as a result. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/SentryPolicyService.java > d112871 > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryConfigValueRequest.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryConfigValueResponse.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java > 65905f5 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > b54e12e > > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift > d9a3913 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java > ff6cff4 > > Diff: https://reviews.apache.org/r/27161/diff/ > > > Testing > ------- > > Unit tests added and run successfully. > > > Thanks, > > Mike Yoder > >
