----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57570/#review170064 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java Lines 181 (patched) <https://reviews.apache.org/r/57570/#comment242838> There is no documentation in this class about the parent-child relationships - since you just looked at this - can you add some text either here or at the class level explaining what is a parent and what is a child? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java Lines 182 (patched) <https://reviews.apache.org/r/57570/#comment242839> What does it mean for path elements to point to the direct parent? May be you can put some example here? Also, from the code it looks like the elements are ordered in a specific way - can you document this as well and/or show example? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java Lines 184 (patched) <https://reviews.apache.org/r/57570/#comment242842> From the code it returns the parent of the *last* element sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java Line 195 (original), 214 (patched) <https://reviews.apache.org/r/57570/#comment242845> Why do you need subList() here? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java Lines 325 (patched) <https://reviews.apache.org/r/57570/#comment242848> Didn't you just check for this in line 319? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java Line 298 (original), 336 (patched) <https://reviews.apache.org/r/57570/#comment242847> This isn't related, but since you are touching this file - why do you call remove() if you know that getChildren() is empty? Can it be that the condition is reversed here by any chance? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java Line 579 (original), 624 (patched) <https://reviews.apache.org/r/57570/#comment242851> formatting - space between // and text sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java Line 587 (original), 629 (patched) <https://reviews.apache.org/r/57570/#comment242853> Why do you need subList() here? - Alexander Kolbasov On March 22, 2017, 9:27 p.m., Lei Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57570/ > ----------------------------------------------------------- > > (Updated March 22, 2017, 9:27 p.m.) > > > Review request for sentry, Alexander Kolbasov and Hao Hao. > > > Bugs: SENTRY-1644 > https://issues.apache.org/jira/browse/SENTRY-1644 > > > Repository: sentry > > > Description > ------- > > After rename Hive table with partitions, ACLs on the partition paths > disappear. > > It is caused by the fact that renaming on sentry does not move any > sub-namespace. > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java > 6d2ab23c3f65af5430ea02563e13c4c4c49aa1c6 > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java > 0dee1028ac852b12b109e875b2cbbb0bd2efbe03 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java > d079628a0e549e3b179c7a321f82bcf6b580da43 > > > Diff: https://reviews.apache.org/r/57570/diff/2/ > > > Testing > ------- > > mvn test -Dtest=TestHMSPaths > > > Thanks, > > Lei Xu > >
