You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Vadim Spector <vs...@gmail.com> on 2016/06/11 02:30:36 UTC
Review Request 48576: CR request for SENTRY-1311
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48576/
-----------------------------------------------------------
Review request for sentry, Hao Hao and Sravya Tirukkovalur.
Repository: sentry
Description
-------
SENTRY-1311: Improve usability of URI privileges by supporting mixed use of URIs with and without scheme
Diffs
-----
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java 3b9336c3e0d70b03dd6e2bf27919e2510031addd
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 1606b6d97c6696328906100eb10464fdc2fa19e8
Diff: https://reviews.apache.org/r/48576/diff/
Testing
-------
Thanks,
Vadim Spector
Re: Review Request 48576: SENTRY-1311: Improve usability of URI
privileges by supporting mixed use of URIs with and without scheme
Posted by Vadim Spector <vs...@gmail.com>.
> On June 14, 2016, 8:50 p.m., Hao Hao wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java, line 106
> > <https://reviews.apache.org/r/48576/diff/1/?file=1415444#file1415444line106>
> >
> > I feel it would be more clean to add the logic to use the default authority here instead of at line 78. We would want to ensure makeFullQualifiedURI will return a full qualified URI.
PathUtils has two public versions of impliesURI():
* impliesURI(URI,URI) - the one with my fix. It never calls makeFullQualifiedURI(), so if this method is called directly, fixing makeFullQualifiedURI() won't help.
* impliesURI(String,String) does call makeFullQualifiedURI() and then delegates to impliesURI(URI,URI). If only this method is ever called, it is true that fixing makeFullQualifiedURI() is all we need.
Since both flavors of impliesURI(,) are public, I put the fix into impliesURI(URI,URI)
But I agree that makeFullQualifiedURI() should set all the missing URI elements to the default. Currently it does not, because it uses Path.isAbsoluteAndSchemeAuthorityNull() which returns true only if BOTH scheme and authority are null. I can fix this logic as well, but I still need to keep the implemented fix in impliesURI(URI,URI).
Alternatively, the code can be refactored, so that both flavors of impliesURI() go through the common logic of ensuring fully qualified URI.
Let me know which way you prefer.
- Vadim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48576/#review137580
-----------------------------------------------------------
On June 11, 2016, 2:33 a.m., Vadim Spector wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48576/
> -----------------------------------------------------------
>
> (Updated June 11, 2016, 2:33 a.m.)
>
>
> Review request for sentry, Hao Hao and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-1311
> https://issues.apache.org/jira/browse/SENTRY-1311
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1311: Improve usability of URI privileges by supporting mixed use of URIs with and without scheme
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java 3b9336c3e0d70b03dd6e2bf27919e2510031addd
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 1606b6d97c6696328906100eb10464fdc2fa19e8
>
> Diff: https://reviews.apache.org/r/48576/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vadim Spector
>
>
Re: Review Request 48576: SENTRY-1311: Improve usability of URI
privileges by supporting mixed use of URIs with and without scheme
Posted by Hao Hao <ha...@cloudera.com>.
> On June 14, 2016, 8:50 p.m., Hao Hao wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, line 1392
> > <https://reviews.apache.org/r/48576/diff/1/?file=1415445#file1415445line1392>
> >
> > Probably change the name to testURIsWithoutAuthority?
>
> Vadim Spector wrote:
> Neither of these tests set authority:
>
> testURIsWithAndWithoutScheme: "grant" uses URI w/ scheme, "create table" - w/o scheme; neither sets authority
> testURIsWithoutAndWithScheme: "grant" uses URI w/o scheme, "create table" - w/ scheme; neither sets authority
>
> So, the names only reflect what varies between "grant" and "create table", which is the scheme. If we want the name to also reflect what stays the same,
> i.e. missing authority, I'm fine with it, but then (a) both names should be changed for consistency, and (b) they should still reflect whether the scheme is set. It could be something like:
>
> testURIsWithAndWithoutSchemeNoAuthority
> testURIsWithoutAndWithSchemeNoAuthority
>
> If you think it makes sense, I'll fix it.
Yeah, makes sense, probably add some comments to explain a bit each of the test cases may even help better. Thanks!
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48576/#review137580
-----------------------------------------------------------
On June 11, 2016, 2:33 a.m., Vadim Spector wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48576/
> -----------------------------------------------------------
>
> (Updated June 11, 2016, 2:33 a.m.)
>
>
> Review request for sentry, Hao Hao and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-1311
> https://issues.apache.org/jira/browse/SENTRY-1311
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1311: Improve usability of URI privileges by supporting mixed use of URIs with and without scheme
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java 3b9336c3e0d70b03dd6e2bf27919e2510031addd
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 1606b6d97c6696328906100eb10464fdc2fa19e8
>
> Diff: https://reviews.apache.org/r/48576/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vadim Spector
>
>
Re: Review Request 48576: SENTRY-1311: Improve usability of URI
privileges by supporting mixed use of URIs with and without scheme
Posted by Hao Hao <ha...@cloudera.com>.
> On June 14, 2016, 8:50 p.m., Hao Hao wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java, line 106
> > <https://reviews.apache.org/r/48576/diff/1/?file=1415444#file1415444line106>
> >
> > I feel it would be more clean to add the logic to use the default authority here instead of at line 78. We would want to ensure makeFullQualifiedURI will return a full qualified URI.
>
> Vadim Spector wrote:
> PathUtils has two public versions of impliesURI():
>
> * impliesURI(URI,URI) - the one with my fix. It never calls makeFullQualifiedURI(), so if this method is called directly, fixing makeFullQualifiedURI() won't help.
>
> * impliesURI(String,String) does call makeFullQualifiedURI() and then delegates to impliesURI(URI,URI). If only this method is ever called, it is true that fixing makeFullQualifiedURI() is all we need.
>
> Since both flavors of impliesURI(,) are public, I put the fix into impliesURI(URI,URI)
>
> But I agree that makeFullQualifiedURI() should set all the missing URI elements to the default. Currently it does not, because it uses Path.isAbsoluteAndSchemeAuthorityNull() which returns true only if BOTH scheme and authority are null. I can fix this logic as well, but I still need to keep the implemented fix in impliesURI(URI,URI).
>
> Alternatively, the code can be refactored, so that both flavors of impliesURI() go through the common logic of ensuring fully qualified URI.
>
> Let me know which way you prefer.
I would prefer to refactor the code so that we only have one common logic to maintian. Thanks, Vadim!
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48576/#review137580
-----------------------------------------------------------
On June 11, 2016, 2:33 a.m., Vadim Spector wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48576/
> -----------------------------------------------------------
>
> (Updated June 11, 2016, 2:33 a.m.)
>
>
> Review request for sentry, Hao Hao and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-1311
> https://issues.apache.org/jira/browse/SENTRY-1311
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1311: Improve usability of URI privileges by supporting mixed use of URIs with and without scheme
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java 3b9336c3e0d70b03dd6e2bf27919e2510031addd
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 1606b6d97c6696328906100eb10464fdc2fa19e8
>
> Diff: https://reviews.apache.org/r/48576/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vadim Spector
>
>
Re: Review Request 48576: SENTRY-1311: Improve usability of URI
privileges by supporting mixed use of URIs with and without scheme
Posted by Vadim Spector <vs...@gmail.com>.
> On June 14, 2016, 8:50 p.m., Hao Hao wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, line 1392
> > <https://reviews.apache.org/r/48576/diff/1/?file=1415445#file1415445line1392>
> >
> > Probably change the name to testURIsWithoutAuthority?
Neither of these tests set authority:
testURIsWithAndWithoutScheme: "grant" uses URI w/ scheme, "create table" - w/o scheme; neither sets authority
testURIsWithoutAndWithScheme: "grant" uses URI w/o scheme, "create table" - w/ scheme; neither sets authority
So, the names only reflect what varies between "grant" and "create table", which is the scheme. If we want the name to also reflect what stays the same,
i.e. missing authority, I'm fine with it, but then (a) both names should be changed for consistency, and (b) they should still reflect whether the scheme is set. It could be something like:
testURIsWithAndWithoutSchemeNoAuthority
testURIsWithoutAndWithSchemeNoAuthority
If you think it makes sense, I'll fix it.
- Vadim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48576/#review137580
-----------------------------------------------------------
On June 11, 2016, 2:33 a.m., Vadim Spector wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48576/
> -----------------------------------------------------------
>
> (Updated June 11, 2016, 2:33 a.m.)
>
>
> Review request for sentry, Hao Hao and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-1311
> https://issues.apache.org/jira/browse/SENTRY-1311
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1311: Improve usability of URI privileges by supporting mixed use of URIs with and without scheme
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java 3b9336c3e0d70b03dd6e2bf27919e2510031addd
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 1606b6d97c6696328906100eb10464fdc2fa19e8
>
> Diff: https://reviews.apache.org/r/48576/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vadim Spector
>
>
Re: Review Request 48576: SENTRY-1311: Improve usability of URI
privileges by supporting mixed use of URIs with and without scheme
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48576/#review137580
-----------------------------------------------------------
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java (line 106)
<https://reviews.apache.org/r/48576/#comment202752>
I feel it would be more clean to add the logic to use the default authority here instead of at line 78. We would want to ensure makeFullQualifiedURI will return a full qualified URI.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java (line 1392)
<https://reviews.apache.org/r/48576/#comment202753>
Probably change the name to testURIsWithoutAuthority?
- Hao Hao
On June 11, 2016, 2:33 a.m., Vadim Spector wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48576/
> -----------------------------------------------------------
>
> (Updated June 11, 2016, 2:33 a.m.)
>
>
> Review request for sentry, Hao Hao and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-1311
> https://issues.apache.org/jira/browse/SENTRY-1311
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1311: Improve usability of URI privileges by supporting mixed use of URIs with and without scheme
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java 3b9336c3e0d70b03dd6e2bf27919e2510031addd
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 1606b6d97c6696328906100eb10464fdc2fa19e8
>
> Diff: https://reviews.apache.org/r/48576/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vadim Spector
>
>
Re: Review Request 48576: SENTRY-1311: Improve usability of URI
privileges by supporting mixed use of URIs with and without scheme
Posted by Vadim Spector <vs...@gmail.com>.
> On June 22, 2016, 1:07 a.m., Hao Hao wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java, line 124
> > <https://reviews.apache.org/r/48576/diff/2/?file=1427009#file1427009line124>
> >
> > Remove?
Done, build successsful, tests passed.
- Vadim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48576/#review138996
-----------------------------------------------------------
On June 22, 2016, 5:12 p.m., Vadim Spector wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48576/
> -----------------------------------------------------------
>
> (Updated June 22, 2016, 5:12 p.m.)
>
>
> Review request for sentry, Hao Hao and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-1311
> https://issues.apache.org/jira/browse/SENTRY-1311
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1311: Improve usability of URI privileges by supporting mixed use of URIs with and without scheme and authority
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java 3b9336c3e0d70b03dd6e2bf27919e2510031addd
> sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestPathUtils.java d7d5e0ad8635bb094b16785b6595fc9bd86df515
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 1606b6d97c6696328906100eb10464fdc2fa19e8
>
> Diff: https://reviews.apache.org/r/48576/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vadim Spector
>
>
Re: Review Request 48576: SENTRY-1311: Improve usability of URI
privileges by supporting mixed use of URIs with and without scheme
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48576/#review138996
-----------------------------------------------------------
Fix it, then Ship it!
Looks good to me. Left minor comments.
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java (line 124)
<https://reviews.apache.org/r/48576/#comment204176>
Remove?
- Hao Hao
On June 21, 2016, 9:52 p.m., Vadim Spector wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48576/
> -----------------------------------------------------------
>
> (Updated June 21, 2016, 9:52 p.m.)
>
>
> Review request for sentry, Hao Hao and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-1311
> https://issues.apache.org/jira/browse/SENTRY-1311
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1311: Improve usability of URI privileges by supporting mixed use of URIs with and without scheme and authority
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java 3b9336c3e0d70b03dd6e2bf27919e2510031addd
> sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestPathUtils.java d7d5e0ad8635bb094b16785b6595fc9bd86df515
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 1606b6d97c6696328906100eb10464fdc2fa19e8
>
> Diff: https://reviews.apache.org/r/48576/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vadim Spector
>
>
Re: Review Request 48576: SENTRY-1311: Improve usability of URI
privileges by supporting mixed use of URIs with and without scheme
Posted by Vadim Spector <vs...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48576/
-----------------------------------------------------------
(Updated June 22, 2016, 5:12 p.m.)
Review request for sentry, Hao Hao and Sravya Tirukkovalur.
Bugs: SENTRY-1311
https://issues.apache.org/jira/browse/SENTRY-1311
Repository: sentry
Description
-------
SENTRY-1311: Improve usability of URI privileges by supporting mixed use of URIs with and without scheme and authority
Diffs (updated)
-----
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java 3b9336c3e0d70b03dd6e2bf27919e2510031addd
sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestPathUtils.java d7d5e0ad8635bb094b16785b6595fc9bd86df515
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 1606b6d97c6696328906100eb10464fdc2fa19e8
Diff: https://reviews.apache.org/r/48576/diff/
Testing
-------
Thanks,
Vadim Spector
Re: Review Request 48576: SENTRY-1311: Improve usability of URI
privileges by supporting mixed use of URIs with and without scheme
Posted by Vadim Spector <vs...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48576/
-----------------------------------------------------------
(Updated June 21, 2016, 9:52 p.m.)
Review request for sentry, Hao Hao and Sravya Tirukkovalur.
Changes
-------
1) implemented shared logic for PathUtils.impliesURI(URI,URI) and PathUtils.impliesURI(String,String) to validate input URIs and set missing scheme and/or authority to the defaults for the default file system
2) better unit test method names in TestHDFSIntegration
3) more unit tests in TestPathUtils.java
Bugs: SENTRY-1311
https://issues.apache.org/jira/browse/SENTRY-1311
Repository: sentry
Description (updated)
-------
SENTRY-1311: Improve usability of URI privileges by supporting mixed use of URIs with and without scheme and authority
Diffs (updated)
-----
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java 3b9336c3e0d70b03dd6e2bf27919e2510031addd
sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestPathUtils.java d7d5e0ad8635bb094b16785b6595fc9bd86df515
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 1606b6d97c6696328906100eb10464fdc2fa19e8
Diff: https://reviews.apache.org/r/48576/diff/
Testing
-------
Thanks,
Vadim Spector
Re: Review Request 48576: SENTRY-1311: Improve usability of URI
privileges by supporting mixed use of URIs with and without scheme
Posted by Vadim Spector <vs...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48576/
-----------------------------------------------------------
(Updated June 11, 2016, 2:33 a.m.)
Review request for sentry, Hao Hao and Sravya Tirukkovalur.
Summary (updated)
-----------------
SENTRY-1311: Improve usability of URI privileges by supporting mixed use of URIs with and without scheme
Bugs: SENTRY-1311
https://issues.apache.org/jira/browse/SENTRY-1311
Repository: sentry
Description
-------
SENTRY-1311: Improve usability of URI privileges by supporting mixed use of URIs with and without scheme
Diffs
-----
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java 3b9336c3e0d70b03dd6e2bf27919e2510031addd
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 1606b6d97c6696328906100eb10464fdc2fa19e8
Diff: https://reviews.apache.org/r/48576/diff/
Testing
-------
Thanks,
Vadim Spector