You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org> on 2018/11/29 17:45:43 UTC
Review Request 69484: SENTRY-2460: Export sentry permission
information to HDFS location
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69484/
-----------------------------------------------------------
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-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/1/
Testing
-------
Added new tests to verify the new behavior added.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 69484: SENTRY-2460: Export sentry permission
information to HDFS location
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69484/#review211112
-----------------------------------------------------------
Ship it!
Ship It!
- Sergio Pena
On Dec. 6, 2018, 9:19 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69484/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2018, 9:19 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/3/
>
>
> Testing
> -------
>
> Added new tests to verify the new behavior added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 69484: SENTRY-2460: Export sentry permission
information to HDFS location
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69484/
-----------------------------------------------------------
(Updated Dec. 6, 2018, 9:19 p.m.)
Review request for sentry and Sergio Pena.
Changes
-------
Addressed review comments.
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 (updated)
-----
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/3/
Changes: https://reviews.apache.org/r/69484/diff/2-3/
Testing
-------
Added new tests to verify the new behavior added.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 69484: SENTRY-2460: Export sentry permission
information to HDFS location
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 69484: SENTRY-2460: Export sentry permission
information to HDFS location
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/69484/diff/1-2/
Testing
-------
Added new tests to verify the new behavior added.
Thanks,
kalyan kumar kalvagadda