----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69352/#review211095 -----------------------------------------------------------
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java Lines 107-108 (patched) <https://reviews.apache.org/r/69352/#comment296007> What if objectTrimmed is empty? Should we add a check and continue with the loop if so? This could happen if the 'objects' object is db=db1,,,,, sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java Lines 108 (patched) <https://reviews.apache.org/r/69352/#comment296010> Shouldn't we support the server as well? Sentry has support for it, so perhaps we should do the same thing for the export commands? sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java Line 106 (original), 110 (patched) <https://reviews.apache.org/r/69352/#comment296008> This may throw an exception if the value is empty. The message would be only 'Value cannot be empty'. Should we catch that and throw a better message specified which key does not have a value? An empty key may throw it too, but if you add the empty check after you trim the object and continue the loop, then keys won't never be empty at this point. sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java Lines 120-123 (patched) <https://reviews.apache.org/r/69352/#comment296009> is the expectation of the method to just log errors instead of throwing an exception? I feel we should make the contract of this method to throw an exception if at least one of the KeyValue are invalid. That helps you do better testing here, and avoid an admin had to go to the logs to find why a certain value was not exported. sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift Line 298 (original), 298 (patched) <https://reviews.apache.org/r/69352/#comment296011> Are you making this incompatible with Sentry 2.1? sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift Lines 299-300 (patched) <https://reviews.apache.org/r/69352/#comment296012> I don't see a usage of these two versions yet. Should we remove them and discuss what is the real intention? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Line 1333 (original), 1332 (patched) <https://reviews.apache.org/r/69352/#comment296013> request.getAuthorizables() may return a null value as this is optional in thrift. - Sergio Pena On Dec. 5, 2018, 11:23 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69352/ > ----------------------------------------------------------- > > (Updated Dec. 5, 2018, 11:23 p.m.) > > > Review request for sentry and Sergio Pena. > > > Bugs: SENTRY-2452 > https://issues.apache.org/jira/browse/SENTRY-2452 > > > Repository: sentry > > > Description > ------- > > TSentryExportMappingDataRequest and TSentryImportMappingDataRequest which are > used to send export and import requests to sentry server should be changed to > be able to send a list authorizables for which permission information has to > be exported/imported. > > These requests should accommodate source and target cluster information as > well. > > > Diffs > ----- > > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java > d8c1061d36ed0b92116f0b2dd5ec820ccb166818 > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegePrincipalType.java > 6eb85217b9229438b9adbd1546a408dc507f1645 > > sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryExportMappingDataRequest.java > 13e57e04dd779825e2986b1e527f4e556271a162 > > sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryImportMappingDataRequest.java > 21f3bdf5e304fe2f49684c999d0aa0e0362320de > > sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryPrivilegesResponse.java > cea868f0524c25bf63cfb7c532829354a280df28 > > sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java > 83393a98df5e10b324f74dc12f10c20bc5f02651 > > sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java > 68d864cfbdf18057d87a65a04af8991292aadccf > > sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift > 3364648dc88acbb8de1cc925fe8c512f34a4b064 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java > b9e3bf2921a0696da639e1b1bee5d83cf2b9cee0 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryServiceImportExport.java > cf1fdab382034c8950da2613ef9b0cf1af912e33 > > > Diff: https://reviews.apache.org/r/69352/diff/3/ > > > Testing > ------- > > Made sure that existiung tests pass. > > > Thanks, > > kalyan kumar kalvagadda > >
