----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45730/#review127041 -----------------------------------------------------------
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java (line 296) <https://reviews.apache.org/r/45730/#comment190175> It would be better to use null to indicate there is no filter. So the exportPolicy would better to handle null and empty filter the same way. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java (line 190) <https://reviews.apache.org/r/45730/#comment190177> String filter Use consistent name as what we choose in thrift request structure. For exaple, objectFilter, objectPath or objectName depends on what finally choosen. Other places for the same parameter should also be consistent. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 910) <https://reviews.apache.org/r/45730/#comment190181> Comments here: 1. dbName: try to use full name if it is not too long: databaseName. 2. Prefer to use null for indication of a parameter has no value. 3. Does Sentry has static utility method for parse this? try use a static method. If there is not, contribute this as one instead embeded into the portion of code. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 916) <https://reviews.apache.org/r/45730/#comment190184> We might not really want to name it "tempKV" here. It is just a local variable. If we ask: do we need to name all local variable as "temp"? NO. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 933) <https://reviews.apache.org/r/45730/#comment190188> If getRoleNameTPrivilegesMap and getGroupNameRoleNamesMap methods are changed according to what I suggested, we don't have to check isEmpty(dbName) or isEmpty(tableName) and sperate the handling. They are the same case. Let the two method handle internally. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceImportExport.java (line 96) <https://reviews.apache.org/r/45730/#comment190189> Use null to indicate NO value for the parameter. The same to other instance below. - Jerry Chen On April 5, 2016, 5:31 a.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45730/ > ----------------------------------------------------------- > > (Updated April 5, 2016, 5:31 a.m.) > > > Review request for sentry. > > > Repository: sentry > > > Description > ------- > > Update Sentry Policy Service for export with specific auth object > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java > 73b0941 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java > de50adb > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java > edc5661 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 8881d82 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceImportExport.java > dbe4a27 > > Diff: https://reviews.apache.org/r/45730/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
