You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Ryan Pridgeon <rn...@cloudera.com> on 2015/11/12 18:42:06 UTC
Review Request 40239: SENTRY-954: removeAclFeature/addAclFeature
should only affect dirs under a prefix which is managed.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40239/
-----------------------------------------------------------
Review request for sentry.
Repository: sentry
Description
-------
SENTRY-954: removeAclFeature/addAclFeature should only affect dirs under a prefix which is managed. I did attempt to clean up SentryAuthorizatinProvider a bit as well which may be a no no. Should be easy to revert these changes without too much hassle
Diffs
-----
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 4d03ba3
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 208c93b
Diff: https://reviews.apache.org/r/40239/diff/
Testing
-------
Added testing to TestSentryAuthorizationProvider.java
Thanks,
Ryan Pridgeon
Re: Review Request 40239: SENTRY-954: removeAclFeature/addAclFeature
should only affect dirs under a prefix which is managed.
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40239/#review106319
-----------------------------------------------------------
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java (line 1122)
<https://reviews.apache.org/r/40239/#comment165052>
Did you get a chance to run the e2e test you added? I applied your pacth and encountered serveral issues.
- Hao Hao
On Nov. 12, 2015, 5:42 p.m., Ryan Pridgeon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40239/
> -----------------------------------------------------------
>
> (Updated Nov. 12, 2015, 5:42 p.m.)
>
>
> Review request for sentry.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-954: removeAclFeature/addAclFeature should only affect dirs under a prefix which is managed. I did attempt to clean up SentryAuthorizatinProvider a bit as well which may be a no no. Should be easy to revert these changes without too much hassle
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 4d03ba3
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 208c93b
>
> Diff: https://reviews.apache.org/r/40239/diff/
>
>
> Testing
> -------
>
> Added testing to TestSentryAuthorizationProvider.java
>
>
> Thanks,
>
> Ryan Pridgeon
>
>
Re: Review Request 40239: SENTRY-954: removeAclFeature/addAclFeature
should only affect dirs under a prefix which is managed.
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40239/#review106307
-----------------------------------------------------------
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 272)
<https://reviews.apache.org/r/40239/#comment165045>
braces +1
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java (line 1134)
<https://reviews.apache.org/r/40239/#comment165043>
Should we use // for comments inside the functions?
- Hao Hao
On Nov. 12, 2015, 5:42 p.m., Ryan Pridgeon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40239/
> -----------------------------------------------------------
>
> (Updated Nov. 12, 2015, 5:42 p.m.)
>
>
> Review request for sentry.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-954: removeAclFeature/addAclFeature should only affect dirs under a prefix which is managed. I did attempt to clean up SentryAuthorizatinProvider a bit as well which may be a no no. Should be easy to revert these changes without too much hassle
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 4d03ba3
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 208c93b
>
> Diff: https://reviews.apache.org/r/40239/diff/
>
>
> Testing
> -------
>
> Added testing to TestSentryAuthorizationProvider.java
>
>
> Thanks,
>
> Ryan Pridgeon
>
>
Re: Review Request 40239: SENTRY-954: removeAclFeature/addAclFeature
should only affect dirs under a prefix which is managed.
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40239/#review106275
-----------------------------------------------------------
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 224)
<https://reviews.apache.org/r/40239/#comment165024>
Coding style: if statements always use braces {}
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 236)
<https://reviews.apache.org/r/40239/#comment165025>
Braces
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 260)
<https://reviews.apache.org/r/40239/#comment165026>
Braces
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 262)
<https://reviews.apache.org/r/40239/#comment165028>
Braces
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 263)
<https://reviews.apache.org/r/40239/#comment165027>
Braces
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 272)
<https://reviews.apache.org/r/40239/#comment165029>
Braces
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 413)
<https://reviews.apache.org/r/40239/#comment165030>
Can we have a constant error message string where we just substitute the operation name?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java (line 32)
<https://reviews.apache.org/r/40239/#comment165032>
It is usually a good practice to not import everything from a package. Can lead to unnecessary conflicts down the road.
While you are here, can you also refactor the isManaged to isInPrefix? I think that wording makes more sense.
- Sravya Tirukkovalur
On Nov. 12, 2015, 5:42 p.m., Ryan Pridgeon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40239/
> -----------------------------------------------------------
>
> (Updated Nov. 12, 2015, 5:42 p.m.)
>
>
> Review request for sentry.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-954: removeAclFeature/addAclFeature should only affect dirs under a prefix which is managed. I did attempt to clean up SentryAuthorizatinProvider a bit as well which may be a no no. Should be easy to revert these changes without too much hassle
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 4d03ba3
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 208c93b
>
> Diff: https://reviews.apache.org/r/40239/diff/
>
>
> Testing
> -------
>
> Added testing to TestSentryAuthorizationProvider.java
>
>
> Thanks,
>
> Ryan Pridgeon
>
>