----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69484/#review211094 -----------------------------------------------------------
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java Line 62 (original), 66 (patched) <https://reviews.apache.org/r/69484/#comment295996> I think this line goes in the resourcePath line, right? sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java Lines 94-96 (patched) <https://reviews.apache.org/r/69484/#comment295999> Does it accept relative paths (especially in the local filesystem)? sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java Line 101 (original), 113 (patched) <https://reviews.apache.org/r/69484/#comment295997> I know this line was already here, but don't you think that printing the contents of the file in the log is unsecure and for big contents the log will be too verbose? Just after the line is logged, the same contents is written to the file, so there is duplicated values in two files. Should we remove the contents from be sent to the logs? sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java Lines 115-118 (patched) <https://reviews.apache.org/r/69484/#comment295998> Is the fileSystem closed if an exception is thrown on the br.write() and br.close()? Should these line be called inside a try-with-resources block? sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java Lines 72 (patched) <https://reviews.apache.org/r/69484/#comment296000> Is this assert needed? I think the createTempDir() throws an exception if the directory is not created, so this is not necessary. sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java Lines 74 (patched) <https://reviews.apache.org/r/69484/#comment296001> Same question, is this assert needed? Don't you just need to call mkddirs() only? sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java Lines 77 (patched) <https://reviews.apache.org/r/69484/#comment296002> Why do we need 2 nodes for testing an export operation? MiniDFS clusters add time on running tests, the less we use them or less resources we ask, the test will be faster. sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java Lines 87 (patched) <https://reviews.apache.org/r/69484/#comment296003> When is baseDir a null in this environment? the setupClass should not create a null baseDir, shouldn't it? sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java Lines 87-92 (patched) <https://reviews.apache.org/r/69484/#comment296004> When is baseDir a null in this environment? the setupClass should not create a null baseDir, shouldn't it? Same question for dfsCluster. sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java Line 174 (original), 224 (patched) <https://reviews.apache.org/r/69484/#comment296005> Change the test case name to testImportExportFromLocalFile() so we understand the difference between this and testImportExportFromHDFS() sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java Lines 272 (patched) <https://reviews.apache.org/r/69484/#comment296006> Isn't it better to run resourcePath.toUri().toString() once and keep the value in a string before starting the tests? It is more readable + the string is not being generated every time it is called. - Sergio Pena On Dec. 5, 2018, 10:18 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69484/ > ----------------------------------------------------------- > > (Updated Dec. 5, 2018, 10:18 p.m.) > > > Review request for sentry and Sergio Pena. > > > Bugs: SENTRY-2460 > https://issues.apache.org/jira/browse/SENTRY-2460 > > > Repository: sentry > > > Description > ------- > > Sentry should be able to export permission information to a HDFS location. > > This can be done using hadoop-common library. > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java > b8ec8a1abadd3b7f99160893c5a4adb2608ea927 > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryPolicyFileFormatter.java > 4f465b3671c1fdd6de7cd0773a29108af40311c8 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java > 5f1e3e916d71c51e19d3d37ba788f902ed6b7e27 > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java > 7baeee17339d29576b841480e600a3cfaf14adf3 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java > 092060c450c6a906850630cb10454737157af5fe > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryServiceImportExport.java > cf1fdab382034c8950da2613ef9b0cf1af912e33 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java > df9299c5df26c891ceebbc59b79dd0f7cd3ceeb4 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java > b048989b42bde0318c6d0fa0e9353a2a59954407 > > > Diff: https://reviews.apache.org/r/69484/diff/2/ > > > Testing > ------- > > Added new tests to verify the new behavior added. > > > Thanks, > > kalyan kumar kalvagadda > >
