You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Szehon Ho <sz...@cloudera.com> on 2014/04/17 02:22:12 UTC
Review Request 20435: HIVE-6916 - Export/import inherit permissions from
parent directory
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20435/
-----------------------------------------------------------
Review request for hive and Brock Noland.
Repository: hive-git
Description
-------
This fixes the CopyTask (used by export/import) to also do the permission inheritance semantics, if the flag is on. Like elsewhere in the code, this is using the HDFS shell because it allows specification of recursion (whereas I could not find it in the HDFS-API).
Diffs
-----
common/src/java/org/apache/hadoop/hive/common/FileUtils.java b36a016
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/TestFolderPermissions.java a635bb0
ql/src/java/org/apache/hadoop/hive/ql/exec/CopyTask.java b429a58
Diff: https://reviews.apache.org/r/20435/diff/
Testing
-------
Added a unit test for the same.
Thanks,
Szehon Ho
Re: Review Request 20435: HIVE-6916 - Export/import inherit permissions from
parent directory
Posted by Xuefu Zhang <xz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20435/#review40797
-----------------------------------------------------------
Ship it!
I think you might need to upload the new patch to the JIRA.
- Xuefu Zhang
On April 18, 2014, 6:33 p.m., Szehon Ho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20435/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 6:33 p.m.)
>
>
> Review request for hive and Brock Noland.
>
>
> Bugs: HIVE-6916
> https://issues.apache.org/jira/browse/HIVE-6916
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> This fixes the CopyTask (used by export/import) to also do the permission inheritance semantics, if the flag is on. Like elsewhere in the code, this is using the HDFS shell because it allows specification of recursion (whereas I could not find it in the HDFS-API).
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/FileUtils.java b36a016
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/TestFolderPermissions.java a635bb0
> ql/src/java/org/apache/hadoop/hive/ql/exec/CopyTask.java b429a58
>
> Diff: https://reviews.apache.org/r/20435/diff/
>
>
> Testing
> -------
>
> Added a unit test for the same.
>
>
> Thanks,
>
> Szehon Ho
>
>
Re: Review Request 20435: HIVE-6916 - Export/import inherit permissions from
parent directory
Posted by Szehon Ho <sz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20435/
-----------------------------------------------------------
(Updated April 18, 2014, 6:33 p.m.)
Review request for hive and Brock Noland.
Changes
-------
Addressing some review comments.
Bugs: HIVE-6916
https://issues.apache.org/jira/browse/HIVE-6916
Repository: hive-git
Description
-------
This fixes the CopyTask (used by export/import) to also do the permission inheritance semantics, if the flag is on. Like elsewhere in the code, this is using the HDFS shell because it allows specification of recursion (whereas I could not find it in the HDFS-API).
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/common/FileUtils.java b36a016
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/TestFolderPermissions.java a635bb0
ql/src/java/org/apache/hadoop/hive/ql/exec/CopyTask.java b429a58
Diff: https://reviews.apache.org/r/20435/diff/
Testing
-------
Added a unit test for the same.
Thanks,
Szehon Ho
Re: Review Request 20435: HIVE-6916 - Export/import inherit permissions from
parent directory
Posted by Szehon Ho <sz...@cloudera.com>.
> On April 18, 2014, 5:13 a.m., Xuefu Zhang wrote:
> > common/src/java/org/apache/hadoop/hive/common/FileUtils.java, line 509
> > <https://reviews.apache.org/r/20435/diff/1/?file=561429#file561429line509>
> >
> > I saw FileSystem.setPermission(Path, FsPermission), which might be what you're looking for. If recursion is the concern, it's probably not too hard to write a util method to do that. In fact you may even be able to find the code from FsShell.java. Just for your consideration.
Yea I did see that. If there's not much advantage of one over another, I would prefer using the shell for simplicity as boils down to using a recursive call like you said. That way code is simpler, and there's already some precedents to use the Shell in other Hive code. What do you think, or do you see some advantage of using the API way?
- Szehon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20435/#review40742
-----------------------------------------------------------
On April 18, 2014, 6:33 p.m., Szehon Ho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20435/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 6:33 p.m.)
>
>
> Review request for hive and Brock Noland.
>
>
> Bugs: HIVE-6916
> https://issues.apache.org/jira/browse/HIVE-6916
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> This fixes the CopyTask (used by export/import) to also do the permission inheritance semantics, if the flag is on. Like elsewhere in the code, this is using the HDFS shell because it allows specification of recursion (whereas I could not find it in the HDFS-API).
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/FileUtils.java b36a016
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/TestFolderPermissions.java a635bb0
> ql/src/java/org/apache/hadoop/hive/ql/exec/CopyTask.java b429a58
>
> Diff: https://reviews.apache.org/r/20435/diff/
>
>
> Testing
> -------
>
> Added a unit test for the same.
>
>
> Thanks,
>
> Szehon Ho
>
>
Re: Review Request 20435: HIVE-6916 - Export/import inherit permissions from
parent directory
Posted by Xuefu Zhang <xz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20435/#review40742
-----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/common/FileUtils.java
<https://reviews.apache.org/r/20435/#comment73883>
dstFS.getFileStatus(dst) is called twice here. We should be able to reduce to one.
common/src/java/org/apache/hadoop/hive/common/FileUtils.java
<https://reviews.apache.org/r/20435/#comment73884>
I saw FileSystem.setPermission(Path, FsPermission), which might be what you're looking for. If recursion is the concern, it's probably not too hard to write a util method to do that. In fact you may even be able to find the code from FsShell.java. Just for your consideration.
- Xuefu Zhang
On April 17, 2014, 4:58 a.m., Szehon Ho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20435/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 4:58 a.m.)
>
>
> Review request for hive and Brock Noland.
>
>
> Bugs: HIVE-6916
> https://issues.apache.org/jira/browse/HIVE-6916
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> This fixes the CopyTask (used by export/import) to also do the permission inheritance semantics, if the flag is on. Like elsewhere in the code, this is using the HDFS shell because it allows specification of recursion (whereas I could not find it in the HDFS-API).
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/FileUtils.java b36a016
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/TestFolderPermissions.java a635bb0
> ql/src/java/org/apache/hadoop/hive/ql/exec/CopyTask.java b429a58
>
> Diff: https://reviews.apache.org/r/20435/diff/
>
>
> Testing
> -------
>
> Added a unit test for the same.
>
>
> Thanks,
>
> Szehon Ho
>
>
Re: Review Request 20435: HIVE-6916 - Export/import inherit permissions from
parent directory
Posted by Szehon Ho <sz...@cloudera.com>.
> On April 18, 2014, 2:55 a.m., Xuefu Zhang wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/CopyTask.java, line 83
> > <https://reviews.apache.org/r/20435/diff/1/?file=561431#file561431line83>
> >
> > It's interesting to note that we already have FileUtil.copy() somewhere, while you created a different version in FileUtils.copy(). Couldn't we enhance existing one instead? Or at least, put those related method close together.
As discussed in JIRA, FileUtil is a HDFS one, and in Hive there is the FileUtils.
- Szehon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20435/#review40736
-----------------------------------------------------------
On April 18, 2014, 6:33 p.m., Szehon Ho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20435/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 6:33 p.m.)
>
>
> Review request for hive and Brock Noland.
>
>
> Bugs: HIVE-6916
> https://issues.apache.org/jira/browse/HIVE-6916
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> This fixes the CopyTask (used by export/import) to also do the permission inheritance semantics, if the flag is on. Like elsewhere in the code, this is using the HDFS shell because it allows specification of recursion (whereas I could not find it in the HDFS-API).
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/FileUtils.java b36a016
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/TestFolderPermissions.java a635bb0
> ql/src/java/org/apache/hadoop/hive/ql/exec/CopyTask.java b429a58
>
> Diff: https://reviews.apache.org/r/20435/diff/
>
>
> Testing
> -------
>
> Added a unit test for the same.
>
>
> Thanks,
>
> Szehon Ho
>
>
Re: Review Request 20435: HIVE-6916 - Export/import inherit permissions from
parent directory
Posted by Xuefu Zhang <xz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20435/#review40736
-----------------------------------------------------------
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/TestFolderPermissions.java
<https://reviews.apache.org/r/20435/#comment73880>
Minor Nit, could you rename the method to make more explicit?
ql/src/java/org/apache/hadoop/hive/ql/exec/CopyTask.java
<https://reviews.apache.org/r/20435/#comment73879>
It's interesting to note that we already have FileUtil.copy() somewhere, while you created a different version in FileUtils.copy(). Couldn't we enhance existing one instead? Or at least, put those related method close together.
- Xuefu Zhang
On April 17, 2014, 4:58 a.m., Szehon Ho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20435/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 4:58 a.m.)
>
>
> Review request for hive and Brock Noland.
>
>
> Bugs: HIVE-6916
> https://issues.apache.org/jira/browse/HIVE-6916
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> This fixes the CopyTask (used by export/import) to also do the permission inheritance semantics, if the flag is on. Like elsewhere in the code, this is using the HDFS shell because it allows specification of recursion (whereas I could not find it in the HDFS-API).
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/FileUtils.java b36a016
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/TestFolderPermissions.java a635bb0
> ql/src/java/org/apache/hadoop/hive/ql/exec/CopyTask.java b429a58
>
> Diff: https://reviews.apache.org/r/20435/diff/
>
>
> Testing
> -------
>
> Added a unit test for the same.
>
>
> Thanks,
>
> Szehon Ho
>
>
Re: Review Request 20435: HIVE-6916 - Export/import inherit permissions from
parent directory
Posted by Szehon Ho <sz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20435/
-----------------------------------------------------------
(Updated April 17, 2014, 4:58 a.m.)
Review request for hive and Brock Noland.
Bugs: HIVE-6916
https://issues.apache.org/jira/browse/HIVE-6916
Repository: hive-git
Description
-------
This fixes the CopyTask (used by export/import) to also do the permission inheritance semantics, if the flag is on. Like elsewhere in the code, this is using the HDFS shell because it allows specification of recursion (whereas I could not find it in the HDFS-API).
Diffs
-----
common/src/java/org/apache/hadoop/hive/common/FileUtils.java b36a016
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/TestFolderPermissions.java a635bb0
ql/src/java/org/apache/hadoop/hive/ql/exec/CopyTask.java b429a58
Diff: https://reviews.apache.org/r/20435/diff/
Testing
-------
Added a unit test for the same.
Thanks,
Szehon Ho