You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Sergey Shelukhin <se...@hortonworks.com> on 2018/02/15 01:28:59 UTC
Review Request 65663: HIVE-18710 extend inheritPerms to ACID in Hive
2.X
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65663/
-----------------------------------------------------------
Review request for hive and Ashutosh Chauhan.
Repository: hive-git
Description
-------
.
Diffs
-----
common/src/java/org/apache/hadoop/hive/common/FileUtils.java 2b7a57bb369f0fc3204157a15dff761c1e73419d
common/src/java/org/apache/hadoop/hive/io/HdfsUtils.java 16fc96eae99705a0625831122afcef3586fe490e
ql/src/java/org/apache/hadoop/hive/ql/Context.java 2ed1c6e00417f713ba8ded16b7591973b7e64271
ql/src/java/org/apache/hadoop/hive/ql/exec/AbstractFileMergeOperator.java 6bba057e764099ff2e6ef974577cb6b42fbecab0
ql/src/java/org/apache/hadoop/hive/ql/exec/CopyTask.java 2683f294f61f5e6e1c553c0095fcc11461229c04
ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java a9d03d060adeaa5cad6bef48a63c048f23819d01
ql/src/java/org/apache/hadoop/hive/ql/exec/JoinOperator.java 02827635873ffc319fca40fefbb3cea0fdc4c418
ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java 5cf2c2bc4869fe3f381fd27366a40564eb9c86c2
ql/src/java/org/apache/hadoop/hive/ql/exec/ReplCopyTask.java d2f9e79cd8366a4fa7541660c41fe06d72922b13
ql/src/java/org/apache/hadoop/hive/ql/exec/SkewJoinHandler.java cec7c1a8deddd45b44d388e732b45b77fd48a68e
ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java 614c29bb15557e28523d17d6438830dc9b9d359d
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java eefa8f756cdcb7d56ed5ff46f4f0bcbfd170a0cf
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorMR.java 5deec4b0f96682dae9b5558ac26060d845044984
Diff: https://reviews.apache.org/r/65663/diff/1/
Testing
-------
Thanks,
Sergey Shelukhin
Re: Review Request 65663: HIVE-18710 extend inheritPerms to ACID in
Hive 2.X
Posted by Sergey Shelukhin <se...@hortonworks.com>.
> On Feb. 15, 2018, 5:38 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/AbstractFileMergeOperator.java
> > Line 225 (original), 227 (patched)
> > <https://reviews.apache.org/r/65663/diff/1/?file=1961243#file1961243line227>
> >
> > inherit perms feature till now is for insert statements, not for concat/merge files. If you want to increase the scope of a feature we shall do it in a seperate jira and limit this jira for acid tables perm inheritance.
Merge can happen as part of normal query execution, so it is needed for full support.
"Fortunately", it's not supported for ACID in 2.X, so I'll just leave a TODO.
> On Feb. 15, 2018, 5:38 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java
> > Line 231 (original), 235 (patched)
> > <https://reviews.apache.org/r/65663/diff/1/?file=1961245#file1961245line235>
> >
> > Permission inheritance is taken care in MoveTask. Why is there a need to do this here?
Nm, this would only be needed for MM tables, not in 2.X
> On Feb. 15, 2018, 5:38 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
> > Lines 2928 (patched)
> > <https://reviews.apache.org/r/65663/diff/1/?file=1961251#file1961251line2928>
> >
> > Please read comment above, this is not needed.
Not sure what you mean. As far as I can tell mvFile does not inherit permissions in the code
> On Feb. 15, 2018, 5:38 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
> > Lines 3181 (patched)
> > <https://reviews.apache.org/r/65663/diff/1/?file=1961251#file1961251line3184>
> >
> > Because feature is for both perms and grp.
But it takes the dest status, and also preserves /source/ group (from the temporary file)
> On Feb. 15, 2018, 5:38 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
> > Line 3182 (original), 3186 (patched)
> > <https://reviews.apache.org/r/65663/diff/1/?file=1961251#file1961251line3189>
> >
> > These are files. No need for recursive here.
Actually for ACID, some cases of union, etc they may not be files. There was a recent bug with DML events about that
> On Feb. 15, 2018, 5:38 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
> > Line 3268 (original), 3278 (patched)
> > <https://reviews.apache.org/r/65663/diff/1/?file=1961251#file1961251line3281>
> >
> > increasing scope of feature.
This is used when loading partitions and tables after the query and as such can easily apply to ACID tables.
lP/lT is called for DP, etc., and it's easier to add it here then to trace all possible paths these calls can take.
- Sergey
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65663/#review197625
-----------------------------------------------------------
On Feb. 15, 2018, 1:28 a.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65663/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2018, 1:28 a.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> .
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/FileUtils.java 2b7a57bb369f0fc3204157a15dff761c1e73419d
> common/src/java/org/apache/hadoop/hive/io/HdfsUtils.java 16fc96eae99705a0625831122afcef3586fe490e
> ql/src/java/org/apache/hadoop/hive/ql/Context.java 2ed1c6e00417f713ba8ded16b7591973b7e64271
> ql/src/java/org/apache/hadoop/hive/ql/exec/AbstractFileMergeOperator.java 6bba057e764099ff2e6ef974577cb6b42fbecab0
> ql/src/java/org/apache/hadoop/hive/ql/exec/CopyTask.java 2683f294f61f5e6e1c553c0095fcc11461229c04
> ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java a9d03d060adeaa5cad6bef48a63c048f23819d01
> ql/src/java/org/apache/hadoop/hive/ql/exec/JoinOperator.java 02827635873ffc319fca40fefbb3cea0fdc4c418
> ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java 5cf2c2bc4869fe3f381fd27366a40564eb9c86c2
> ql/src/java/org/apache/hadoop/hive/ql/exec/ReplCopyTask.java d2f9e79cd8366a4fa7541660c41fe06d72922b13
> ql/src/java/org/apache/hadoop/hive/ql/exec/SkewJoinHandler.java cec7c1a8deddd45b44d388e732b45b77fd48a68e
> ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java 614c29bb15557e28523d17d6438830dc9b9d359d
> ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java eefa8f756cdcb7d56ed5ff46f4f0bcbfd170a0cf
> ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorMR.java 5deec4b0f96682dae9b5558ac26060d845044984
>
>
> Diff: https://reviews.apache.org/r/65663/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 65663: HIVE-18710 extend inheritPerms to ACID in
Hive 2.X
Posted by Ashutosh Chauhan <ha...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65663/#review197625
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/Context.java
Lines 366 (patched)
<https://reviews.apache.org/r/65663/#comment277835>
This is a staging dir... there is no need to inherit perms for temp dir.
ql/src/java/org/apache/hadoop/hive/ql/exec/AbstractFileMergeOperator.java
Line 210 (original), 211 (patched)
<https://reviews.apache.org/r/65663/#comment277836>
whitespace.
ql/src/java/org/apache/hadoop/hive/ql/exec/AbstractFileMergeOperator.java
Line 225 (original), 227 (patched)
<https://reviews.apache.org/r/65663/#comment277837>
inherit perms feature till now is for insert statements, not for concat/merge files. If you want to increase the scope of a feature we shall do it in a seperate jira and limit this jira for acid tables perm inheritance.
ql/src/java/org/apache/hadoop/hive/ql/exec/AbstractFileMergeOperator.java
Line 264 (original), 265 (patched)
<https://reviews.apache.org/r/65663/#comment277838>
inherit perms feature till now is for insert statements, not for concat/merge files. If you want to increase the scope of a feature we shall do it in a seperate jira and limit this jira for acid tables perm inheritance.
ql/src/java/org/apache/hadoop/hive/ql/exec/CopyTask.java
Lines 75 (patched)
<https://reviews.apache.org/r/65663/#comment277839>
inherit perms feature till now is for insert statements, not for load/copy files. If you want to increase the scope of a feature we shall do it in a seperate jira and limit this jira for acid tables perm inheritance.
ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java
Line 231 (original), 235 (patched)
<https://reviews.apache.org/r/65663/#comment277840>
Permission inheritance is taken care in MoveTask. Why is there a need to do this here?
ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java
Line 240 (original), 245 (patched)
<https://reviews.apache.org/r/65663/#comment277841>
Permission inheritance is taken care in MoveTask. Why is there a need to do this here?
ql/src/java/org/apache/hadoop/hive/ql/exec/ReplCopyTask.java
Line 109 (original), 112 (patched)
<https://reviews.apache.org/r/65663/#comment277842>
inherit perms feature till now is for insert statements, not for replication. If you want to increase the scope of a feature we shall do it in a seperate jira and limit this jira for acid tables perm inheritance.
ql/src/java/org/apache/hadoop/hive/ql/exec/SkewJoinHandler.java
Line 323 (original), 324 (patched)
<https://reviews.apache.org/r/65663/#comment277843>
these are temp dir, no need for inheritance here.
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
Lines 2928 (patched)
<https://reviews.apache.org/r/65663/#comment277844>
Please read comment above, this is not needed.
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
Lines 3181 (patched)
<https://reviews.apache.org/r/65663/#comment277845>
Because feature is for both perms and grp.
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
Line 3182 (original), 3186 (patched)
<https://reviews.apache.org/r/65663/#comment277848>
These are files. No need for recursive here.
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
Lines 3214 (patched)
<https://reviews.apache.org/r/65663/#comment277846>
perms are already inherited. No need to call again.
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
Line 3268 (original), 3278 (patched)
<https://reviews.apache.org/r/65663/#comment277847>
increasing scope of feature.
- Ashutosh Chauhan
On Feb. 15, 2018, 1:28 a.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65663/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2018, 1:28 a.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> .
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/FileUtils.java 2b7a57bb369f0fc3204157a15dff761c1e73419d
> common/src/java/org/apache/hadoop/hive/io/HdfsUtils.java 16fc96eae99705a0625831122afcef3586fe490e
> ql/src/java/org/apache/hadoop/hive/ql/Context.java 2ed1c6e00417f713ba8ded16b7591973b7e64271
> ql/src/java/org/apache/hadoop/hive/ql/exec/AbstractFileMergeOperator.java 6bba057e764099ff2e6ef974577cb6b42fbecab0
> ql/src/java/org/apache/hadoop/hive/ql/exec/CopyTask.java 2683f294f61f5e6e1c553c0095fcc11461229c04
> ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java a9d03d060adeaa5cad6bef48a63c048f23819d01
> ql/src/java/org/apache/hadoop/hive/ql/exec/JoinOperator.java 02827635873ffc319fca40fefbb3cea0fdc4c418
> ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java 5cf2c2bc4869fe3f381fd27366a40564eb9c86c2
> ql/src/java/org/apache/hadoop/hive/ql/exec/ReplCopyTask.java d2f9e79cd8366a4fa7541660c41fe06d72922b13
> ql/src/java/org/apache/hadoop/hive/ql/exec/SkewJoinHandler.java cec7c1a8deddd45b44d388e732b45b77fd48a68e
> ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java 614c29bb15557e28523d17d6438830dc9b9d359d
> ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java eefa8f756cdcb7d56ed5ff46f4f0bcbfd170a0cf
> ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorMR.java 5deec4b0f96682dae9b5558ac26060d845044984
>
>
> Diff: https://reviews.apache.org/r/65663/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 65663: HIVE-18710 extend inheritPerms to ACID in
Hive 2.X
Posted by Sergey Shelukhin <se...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65663/
-----------------------------------------------------------
(Updated Feb. 22, 2018, 2:22 a.m.)
Review request for hive and Ashutosh Chauhan.
Repository: hive-git
Description
-------
.
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/common/FileUtils.java 2b7a57bb36
common/src/java/org/apache/hadoop/hive/io/HdfsUtils.java 16fc96eae9
ql/src/java/org/apache/hadoop/hive/ql/exec/AbstractFileMergeOperator.java 6bba057e76
ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java a9d03d060a
ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java 5cf2c2bc48
ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java 614c29bb15
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java eefa8f756c
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorMR.java 5deec4b0f9
Diff: https://reviews.apache.org/r/65663/diff/3/
Changes: https://reviews.apache.org/r/65663/diff/2-3/
Testing
-------
Thanks,
Sergey Shelukhin
Re: Review Request 65663: HIVE-18710 extend inheritPerms to ACID in
Hive 2.X
Posted by Sergey Shelukhin <se...@hortonworks.com>.
> On Feb. 21, 2018, 1:41 a.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java
> > Lines 218 (patched)
> > <https://reviews.apache.org/r/65663/diff/2/?file=1962941#file1962941line218>
> >
> > This is a short lived temp dir, there is no need to inherit perms for this. Actual partition dir perms inherit will be handled in move task.
Sorry, had MM tables in mind, this doesn't apply in 2.x
> On Feb. 21, 2018, 1:41 a.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
> > Lines 2927 (patched)
> > <https://reviews.apache.org/r/65663/diff/2/?file=1962945#file1962945line2927>
> >
> > Please see the comment above in this method. This is not required.
See the previous review iteration where I left an open item. I am not sure how that comment applies :)
- Sergey
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65663/#review197837
-----------------------------------------------------------
On Feb. 20, 2018, 8:33 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65663/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2018, 8:33 p.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> .
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/FileUtils.java 2b7a57bb36
> common/src/java/org/apache/hadoop/hive/io/HdfsUtils.java 16fc96eae9
> ql/src/java/org/apache/hadoop/hive/ql/exec/AbstractFileMergeOperator.java 6bba057e76
> ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java a9d03d060a
> ql/src/java/org/apache/hadoop/hive/ql/exec/JoinOperator.java 0282763587
> ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java 5cf2c2bc48
> ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java 614c29bb15
> ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java eefa8f756c
> ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorMR.java 5deec4b0f9
>
>
> Diff: https://reviews.apache.org/r/65663/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 65663: HIVE-18710 extend inheritPerms to ACID in
Hive 2.X
Posted by Ashutosh Chauhan <ha...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65663/#review197837
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java
Lines 218 (patched)
<https://reviews.apache.org/r/65663/#comment278123>
This is a short lived temp dir, there is no need to inherit perms for this. Actual partition dir perms inherit will be handled in move task.
ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java
Line 1155 (original), 1161 (patched)
<https://reviews.apache.org/r/65663/#comment278124>
There is no need for this. This method is called only in operator tree to either create temp dirs or files within temp dirs. In either case inheriting from parent is useless. Move Task sets permissions correctly. No need for this change.
ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java
Line 1167 (original), 1172 (patched)
<https://reviews.apache.org/r/65663/#comment278125>
There is no need for this. This method is called only in operator tree to either create temp dirs or files within temp dirs. In either case inheriting from parent is useless. Move Task sets permissions correctly. No need for this change.
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
Lines 2927 (patched)
<https://reviews.apache.org/r/65663/#comment278126>
Please see the comment above in this method. This is not required.
- Ashutosh Chauhan
On Feb. 20, 2018, 8:33 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65663/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2018, 8:33 p.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> .
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/FileUtils.java 2b7a57bb36
> common/src/java/org/apache/hadoop/hive/io/HdfsUtils.java 16fc96eae9
> ql/src/java/org/apache/hadoop/hive/ql/exec/AbstractFileMergeOperator.java 6bba057e76
> ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java a9d03d060a
> ql/src/java/org/apache/hadoop/hive/ql/exec/JoinOperator.java 0282763587
> ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java 5cf2c2bc48
> ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java 614c29bb15
> ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java eefa8f756c
> ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorMR.java 5deec4b0f9
>
>
> Diff: https://reviews.apache.org/r/65663/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 65663: HIVE-18710 extend inheritPerms to ACID in
Hive 2.X
Posted by Sergey Shelukhin <se...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65663/
-----------------------------------------------------------
(Updated Feb. 20, 2018, 8:33 p.m.)
Review request for hive and Ashutosh Chauhan.
Repository: hive-git
Description
-------
.
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/common/FileUtils.java 2b7a57bb36
common/src/java/org/apache/hadoop/hive/io/HdfsUtils.java 16fc96eae9
ql/src/java/org/apache/hadoop/hive/ql/exec/AbstractFileMergeOperator.java 6bba057e76
ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java a9d03d060a
ql/src/java/org/apache/hadoop/hive/ql/exec/JoinOperator.java 0282763587
ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java 5cf2c2bc48
ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java 614c29bb15
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java eefa8f756c
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorMR.java 5deec4b0f9
Diff: https://reviews.apache.org/r/65663/diff/2/
Changes: https://reviews.apache.org/r/65663/diff/1-2/
Testing
-------
Thanks,
Sergey Shelukhin