Re: Review Request 46077: SENTRY-1184: Clean up HMSPaths.renameAuthzObject

[Hao Hao]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46077/#review128579
---


Fix it, then Ship it!




Ship It!


sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
 (line 914)


Remove the space.


- Hao Hao


On April 12, 2016, 11:22 p.m., Sravya Tirukkovalur wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46077/
> ---
> 
> (Updated April 12, 2016, 11:22 p.m.)
> 
> 
> Review request for sentry and Hao Hao.
> 
> 
> Bugs: SENTRY-1184
> https://issues.apache.org/jira/browse/SENTRY-1184
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently HMSPaths.renameAuthzObject is being used for multiple cases and 
> logic is not being handled well causing expected behaviors to throw 
> exceptions. This patch makes following changes:
> 
> If oldName == newName, oldPath != newPath. This is treated as regular 
> newName.add(newPath), newName.delete(oldPath), so renameAuthz is not called 
> for this case. Example: Alter table partition rename to partition
> If oldName != newName renameAuthz is called which does the following:
> - If oldPath == newPath =>new_table.add(new_path), 
> new_table.add(old_table_partition_paths), old_table.dropAllPaths. Example: 
> Rename external table
> - oldPath != newPath => new_table.add(new_path), old_table.dropAllPaths. 
> Example: Rename managed table
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  ceb1da80fef393a548f2b88343b1647fdec76783 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  8fc5470088853e639e39a35fe930ca805f8ff08e 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  bb74779bff7c40202702f6e099fba45c47beecd3 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java
>  a5bc313e6ebefe4b50a78f76a1d7e4222de7d69d 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
>  4799d36f55308abb2c78e03e4807fa4ee8d0a79a 
> 
> Diff: https://reviews.apache.org/r/46077/diff/
> 
> 
> Testing
> ---
> 
> Added new unit and e2e test cases
> 
> 
> Thanks,
> 
> Sravya Tirukkovalur
> 
>

Re: Review Request 46077: SENTRY-1184: Clean up HMSPaths.renameAuthzObject

[Hao Hao]

> On April 12, 2016, 11:22 p.m., Sravya Tirukkovalur wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java,
> >  line 567
> > 
> >
> > By managed table I mean, when table rename happens even the underlying 
> > files are moved, so old paths do not exist any more. I can add a detailed 
> > comment if that helps but the nomenclature is coming from Hive.

Thanks a lot! That would be helpful.


- Hao


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46077/#review128527
---


On April 12, 2016, 11:22 p.m., Sravya Tirukkovalur wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46077/
> ---
> 
> (Updated April 12, 2016, 11:22 p.m.)
> 
> 
> Review request for sentry and Hao Hao.
> 
> 
> Bugs: SENTRY-1184
> https://issues.apache.org/jira/browse/SENTRY-1184
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently HMSPaths.renameAuthzObject is being used for multiple cases and 
> logic is not being handled well causing expected behaviors to throw 
> exceptions. This patch makes following changes:
> 
> If oldName == newName, oldPath != newPath. This is treated as regular 
> newName.add(newPath), newName.delete(oldPath), so renameAuthz is not called 
> for this case. Example: Alter table partition rename to partition
> If oldName != newName renameAuthz is called which does the following:
> - If oldPath == newPath =>new_table.add(new_path), 
> new_table.add(old_table_partition_paths), old_table.dropAllPaths. Example: 
> Rename external table
> - oldPath != newPath => new_table.add(new_path), old_table.dropAllPaths. 
> Example: Rename managed table
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  ceb1da80fef393a548f2b88343b1647fdec76783 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  8fc5470088853e639e39a35fe930ca805f8ff08e 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  bb74779bff7c40202702f6e099fba45c47beecd3 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java
>  a5bc313e6ebefe4b50a78f76a1d7e4222de7d69d 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
>  4799d36f55308abb2c78e03e4807fa4ee8d0a79a 
> 
> Diff: https://reviews.apache.org/r/46077/diff/
> 
> 
> Testing
> ---
> 
> Added new unit and e2e test cases
> 
> 
> Thanks,
> 
> Sravya Tirukkovalur
> 
>

Re: Review Request 46077: SENTRY-1184: Clean up HMSPaths.renameAuthzObject

[Hao Hao]

> On April 12, 2016, 11:22 p.m., Sravya Tirukkovalur wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java,
> >  line 263
> > 
> >
> > Log is actually added here. Am I missing anything?

Sorry, Sravya, I was accidently thinking as the opposite, I have dropped the 
comments.


- Hao


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46077/#review128527
---


On April 12, 2016, 11:22 p.m., Sravya Tirukkovalur wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46077/
> ---
> 
> (Updated April 12, 2016, 11:22 p.m.)
> 
> 
> Review request for sentry and Hao Hao.
> 
> 
> Bugs: SENTRY-1184
> https://issues.apache.org/jira/browse/SENTRY-1184
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently HMSPaths.renameAuthzObject is being used for multiple cases and 
> logic is not being handled well causing expected behaviors to throw 
> exceptions. This patch makes following changes:
> 
> If oldName == newName, oldPath != newPath. This is treated as regular 
> newName.add(newPath), newName.delete(oldPath), so renameAuthz is not called 
> for this case. Example: Alter table partition rename to partition
> If oldName != newName renameAuthz is called which does the following:
> - If oldPath == newPath =>new_table.add(new_path), 
> new_table.add(old_table_partition_paths), old_table.dropAllPaths. Example: 
> Rename external table
> - oldPath != newPath => new_table.add(new_path), old_table.dropAllPaths. 
> Example: Rename managed table
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  ceb1da80fef393a548f2b88343b1647fdec76783 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  8fc5470088853e639e39a35fe930ca805f8ff08e 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  bb74779bff7c40202702f6e099fba45c47beecd3 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java
>  a5bc313e6ebefe4b50a78f76a1d7e4222de7d69d 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
>  4799d36f55308abb2c78e03e4807fa4ee8d0a79a 
> 
> Diff: https://reviews.apache.org/r/46077/diff/
> 
> 
> Testing
> ---
> 
> Added new unit and e2e test cases
> 
> 
> Thanks,
> 
> Sravya Tirukkovalur
> 
>

Re: Review Request 46077: SENTRY-1184: Clean up HMSPaths.renameAuthzObject

[Sravya Tirukkovalur]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46077/#review128527
---




sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
 (line 263)


Log is actually added here. Am I missing anything?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
 (line 566)


By managed table I mean, when table rename happens even the underlying 
files are moved, so old paths do not exist any more. I can add a detailed 
comment if that helps but the nomenclature is coming from Hive.



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
 (line 624)


table rename always means oldName !=newName


- Sravya Tirukkovalur


On April 12, 2016, 7:45 p.m., Sravya Tirukkovalur wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46077/
> ---
> 
> (Updated April 12, 2016, 7:45 p.m.)
> 
> 
> Review request for sentry and Hao Hao.
> 
> 
> Bugs: SENTRY-1184
> https://issues.apache.org/jira/browse/SENTRY-1184
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently HMSPaths.renameAuthzObject is being used for multiple cases and 
> logic is not being handled well causing expected behaviors to throw 
> exceptions. This patch makes following changes:
> 
> If oldName == newName, oldPath != newPath. This is treated as regular 
> newName.add(newPath), newName.delete(oldPath), so renameAuthz is not called 
> for this case. Example: Alter table partition rename to partition
> If oldName != newName renameAuthz is called which does the following:
> - If oldPath == newPath =>new_table.add(new_path), 
> new_table.add(old_table_partition_paths), old_table.dropAllPaths. Example: 
> Rename external table
> - oldPath != newPath => new_table.add(new_path), old_table.dropAllPaths. 
> Example: Rename managed table
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  ceb1da80fef393a548f2b88343b1647fdec76783 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  8fc5470088853e639e39a35fe930ca805f8ff08e 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  bb74779bff7c40202702f6e099fba45c47beecd3 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java
>  a5bc313e6ebefe4b50a78f76a1d7e4222de7d69d 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
>  4799d36f55308abb2c78e03e4807fa4ee8d0a79a 
> 
> Diff: https://reviews.apache.org/r/46077/diff/
> 
> 
> Testing
> ---
> 
> Added new unit and e2e test cases
> 
> 
> Thanks,
> 
> Sravya Tirukkovalur
> 
>

Re: Review Request 46077: SENTRY-1184: Clean up HMSPaths.renameAuthzObject

[Hao Hao]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46077/#review128507
---




sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
 (line 263)


Why the log is removed?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
 (line 566)


rename external table instead of rename managed table to make the steps 
more clear in the comments?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
 (line 612)


Added "old_table.dropAllPaths" as the comments before this line?



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
 (line 624)


Make it more specific to Verify when oldName != newName and oldPath != 
newPath?



sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java
 (line 106)


Seems like this is a behavior changes for parition that does not have path 
that map to it? Not sure why the old behavior will need explicit set location? 
Also do we need to add a test case for a location that maps to a parition 
explictly and another hive object that maps to that location. And see after 
rename there are still one objects assocaite with that?


- Hao Hao


On April 12, 2016, 7:45 p.m., Sravya Tirukkovalur wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46077/
> ---
> 
> (Updated April 12, 2016, 7:45 p.m.)
> 
> 
> Review request for sentry and Hao Hao.
> 
> 
> Bugs: SENTRY-1184
> https://issues.apache.org/jira/browse/SENTRY-1184
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently HMSPaths.renameAuthzObject is being used for multiple cases and 
> logic is not being handled well causing expected behaviors to throw 
> exceptions. This patch makes following changes:
> 
> If oldName == newName, oldPath != newPath. This is treated as regular 
> newName.add(newPath), newName.delete(oldPath), so renameAuthz is not called 
> for this case. Example: Alter table partition rename to partition
> If oldName != newName renameAuthz is called which does the following:
> - If oldPath == newPath =>new_table.add(new_path), 
> new_table.add(old_table_partition_paths), old_table.dropAllPaths. Example: 
> Rename external table
> - oldPath != newPath => new_table.add(new_path), old_table.dropAllPaths. 
> Example: Rename managed table
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  ceb1da80fef393a548f2b88343b1647fdec76783 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  8fc5470088853e639e39a35fe930ca805f8ff08e 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  bb74779bff7c40202702f6e099fba45c47beecd3 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java
>  a5bc313e6ebefe4b50a78f76a1d7e4222de7d69d 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
>  4799d36f55308abb2c78e03e4807fa4ee8d0a79a 
> 
> Diff: https://reviews.apache.org/r/46077/diff/
> 
> 
> Testing
> ---
> 
> Added new unit and e2e test cases
> 
> 
> Thanks,
> 
> Sravya Tirukkovalur
> 
>