Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-12-12 Thread Alexander Kolbasov 


> On Dec. 12, 2017, 1:26 a.m., Alexander Kolbasov wrote:
> > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
> > Line 57 (original), 56 (patched)
> > 
> >
> > Not necessarily as part of this fix it would be great to have tesst 
> > that not only check counts but actually verify the content of the returned 
> > values.
> 
> Arjun Mishra wrote:
> Sasha, did you mean for it to be "Fix It"? I chose not to make changes to 
> this test cos the purpose of the ticket was to eliminate any references to 
> retrieveFullPathsImage() method. If you meant for this to be "Fix It" please 
> let me know, else I will ask for someone to commit this.

No, I don't think you need to fix it as part of this change, but it would be 
good to add tests later that test the actual content of the snapshot.


- Alexander


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


On Dec. 11, 2017, 9:02 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63596/
> ---
> 
> (Updated Dec. 11, 2017, 9:02 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, Na 
> Li, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The old retrieveFullPathsImage() method in SentryStore is no longer used by 
> actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
> instead. It was preserved because it is used by test which now doesn't make 
> much sense.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  b355630e7 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  f09d1b228 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  c730a03a5 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  24b11f724 
> 
> 
> Diff: https://reviews.apache.org/r/63596/diff/6/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestSentryHDFSServiceProcessor
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestImageRetriever.java
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-12-12 Thread Arjun Mishra via Review Board 


> On Dec. 12, 2017, 1:26 a.m., Alexander Kolbasov wrote:
> > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
> > Line 57 (original), 56 (patched)
> > 
> >
> > Not necessarily as part of this fix it would be great to have tesst 
> > that not only check counts but actually verify the content of the returned 
> > values.

Sasha, did you mean for it to be "Fix It"? I chose not to make changes to this 
test cos the purpose of the ticket was to eliminate any references to 
retrieveFullPathsImage() method. If you meant for this to be "Fix It" please 
let me know, else I will ask for someone to commit this.


- Arjun


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


On Dec. 11, 2017, 9:02 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63596/
> ---
> 
> (Updated Dec. 11, 2017, 9:02 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, Na 
> Li, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The old retrieveFullPathsImage() method in SentryStore is no longer used by 
> actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
> instead. It was preserved because it is used by test which now doesn't make 
> much sense.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  b355630e7 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  f09d1b228 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  c730a03a5 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  24b11f724 
> 
> 
> Diff: https://reviews.apache.org/r/63596/diff/6/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestSentryHDFSServiceProcessor
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestImageRetriever.java
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-12-11 Thread Alexander Kolbasov 

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


Ship it!




Ship It!


sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
Line 57 (original), 56 (patched)


Not necessarily as part of this fix it would be great to have tesst that 
not only check counts but actually verify the content of the returned values.


- Alexander Kolbasov


On Dec. 11, 2017, 9:02 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63596/
> ---
> 
> (Updated Dec. 11, 2017, 9:02 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, Na 
> Li, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The old retrieveFullPathsImage() method in SentryStore is no longer used by 
> actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
> instead. It was preserved because it is used by test which now doesn't make 
> much sense.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  b355630e7 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  f09d1b228 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  c730a03a5 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  24b11f724 
> 
> 
> Diff: https://reviews.apache.org/r/63596/diff/6/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestSentryHDFSServiceProcessor
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestImageRetriever.java
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-12-11 Thread Arjun Mishra via Review Board 


> On Nov. 30, 2017, 3:09 p.m., Xinran Tinney wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 3574-3580 (patched)
> > 
> >
> > Since this part is repeating through multiple files, is it helpful to 
> > create a function and call the function?

The purpose of buildPathsImageMap() method was to do that. The resof it won't 
make sense since all calls are pass by value. I could create a private class 
but I don't see a need for that


- Arjun


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


On Nov. 29, 2017, 5:23 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63596/
> ---
> 
> (Updated Nov. 29, 2017, 5:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, Na 
> Li, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The old retrieveFullPathsImage() method in SentryStore is no longer used by 
> actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
> instead. It was preserved because it is used by test which now doesn't make 
> much sense.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  b355630e7 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  f09d1b228 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  24b11f724 
> 
> 
> Diff: https://reviews.apache.org/r/63596/diff/5/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestSentryHDFSServiceProcessor
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestImageRetriever.java
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-12-11 Thread Arjun Mishra via Review Board 

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

(Updated Dec. 11, 2017, 9:02 p.m.)


Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, Na Li, 
Sergio Pena, and Vadim Spector.


Changes
---

Minor refactoring


Repository: sentry


Description
---

The old retrieveFullPathsImage() method in SentryStore is no longer used by 
actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
instead. It was preserved because it is used by test which now doesn't make 
much sense.


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
 b355630e7 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
 f09d1b228 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 c730a03a5 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 24b11f724 


Diff: https://reviews.apache.org/r/63596/diff/6/

Changes: https://reviews.apache.org/r/63596/diff/5-6/


Testing
---

mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
-Dtest=TestSentryHDFSServiceProcessor
mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
-Dtest=TestImageRetriever.java


Thanks,

Arjun Mishra

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-30 Thread Xinran Tinney 

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




sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3574-3580 (patched)


Since this part is repeating through multiple files, is it helpful to 
create a function and call the function?


- Xinran Tinney


On Nov. 29, 2017, 5:23 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63596/
> ---
> 
> (Updated Nov. 29, 2017, 5:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, Na 
> Li, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The old retrieveFullPathsImage() method in SentryStore is no longer used by 
> actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
> instead. It was preserved because it is used by test which now doesn't make 
> much sense.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  b355630e7 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  f09d1b228 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  24b11f724 
> 
> 
> Diff: https://reviews.apache.org/r/63596/diff/5/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestSentryHDFSServiceProcessor
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestImageRetriever.java
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-29 Thread Arjun Mishra via Review Board 

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

(Updated Nov. 29, 2017, 5:23 p.m.)


Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, Na Li, 
Sergio Pena, and Vadim Spector.


Changes
---

Accidentally added previous version


Repository: sentry


Description
---

The old retrieveFullPathsImage() method in SentryStore is no longer used by 
actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
instead. It was preserved because it is used by test which now doesn't make 
much sense.


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
 b355630e7 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
 f09d1b228 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 f32a745ed 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 24b11f724 


Diff: https://reviews.apache.org/r/63596/diff/5/

Changes: https://reviews.apache.org/r/63596/diff/4-5/


Testing
---

mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
-Dtest=TestSentryHDFSServiceProcessor
mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
-Dtest=TestImageRetriever.java


Thanks,

Arjun Mishra

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-29 Thread Arjun Mishra via Review Board 

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

(Updated Nov. 29, 2017, 5:22 p.m.)


Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, Na Li, 
Sergio Pena, and Vadim Spector.


Repository: sentry


Description
---

The old retrieveFullPathsImage() method in SentryStore is no longer used by 
actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
instead. It was preserved because it is used by test which now doesn't make 
much sense.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 f32a745ed 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 c1471d118 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 eccb40fb6 


Diff: https://reviews.apache.org/r/63596/diff/4/

Changes: https://reviews.apache.org/r/63596/diff/3-4/


Testing
---

mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
-Dtest=TestSentryHDFSServiceProcessor
mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
-Dtest=TestImageRetriever.java


Thanks,

Arjun Mishra

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-29 Thread Arjun Mishra via Review Board 

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

(Updated Nov. 29, 2017, 5:20 p.m.)


Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, Na Li, 
Sergio Pena, and Vadim Spector.


Changes
---

Updated patch based on Sergio's feedback


Repository: sentry


Description
---

The old retrieveFullPathsImage() method in SentryStore is no longer used by 
actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
instead. It was preserved because it is used by test which now doesn't make 
much sense.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 f32a745ed 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 c1471d118 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 eccb40fb6 


Diff: https://reviews.apache.org/r/63596/diff/3/

Changes: https://reviews.apache.org/r/63596/diff/2-3/


Testing
---

mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
-Dtest=TestSentryHDFSServiceProcessor
mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
-Dtest=TestImageRetriever.java


Thanks,

Arjun Mishra

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-28 Thread Vadim Spector via Review Board 


> On Nov. 28, 2017, 10:16 p.m., Vadim Spector wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Line 2514 (original), 2513 (patched)
> > 
> >
> > So, what are our final assumptions about leading and trailing hashes in 
> > hdfs paths at the point of adding them to Sentry store? Do we assume they 
> > only have leading slashes? Can they be multiple slashes or single-only? 
> > What about trailing slashes - none, single-only, multiple ok?
> > 
> > I start thinking that sentry store should be taking hdfs paths with 
> > "bad" slashes and canoinicalize them internally. I like the idea of a 
> > contract between Sentry components in regards to hdfs paths; we keep 
> > talking about, but the truth is contracts can be inadvertantly broken by 
> > code changes. Canonicalizing paths seems like a cheap operation, so I think 
> > that Sentry store should be fine accepting non-canonicalized paths - in 
> > fact, we should do just that and test the resuts by passing 
> > non-canonicalized paths and make sure canonicalization happened.
> > 
> > Would like others to weigh in.
> 
> Vadim Spector wrote:
> No matter what we decide, should probably document those assuimptions in 
> the test code, if only in couple of sentences.
> 
> Arjun Mishra wrote:
> Vadim, the newer version doesn't have the specific line you have 
> commented against. In the new version all the test cases are unchanged with 
> respect to the inputs that are being fed

Does not really matter, imo. The point is, we switch to another API, and change 
tests accordingly. It may be a good time to clearly state our assumptions (and 
probably even change them), to avoid future problems. We know how hard it is to 
debug issues involving incorrect hdfs path parsing; cheaper to harden our code 
right now, imo.


- Vadim


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


On Nov. 27, 2017, 6:03 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63596/
> ---
> 
> (Updated Nov. 27, 2017, 6:03 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, Na 
> Li, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The old retrieveFullPathsImage() method in SentryStore is no longer used by 
> actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
> instead. It was preserved because it is used by test which now doesn't make 
> much sense.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  b355630e7 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  f09d1b228 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  24b11f724 
> 
> 
> Diff: https://reviews.apache.org/r/63596/diff/2/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestSentryHDFSServiceProcessor
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestImageRetriever.java
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-28 Thread Arjun Mishra via Review Board 


> On Nov. 28, 2017, 10:16 p.m., Vadim Spector wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Line 2514 (original), 2513 (patched)
> > 
> >
> > So, what are our final assumptions about leading and trailing hashes in 
> > hdfs paths at the point of adding them to Sentry store? Do we assume they 
> > only have leading slashes? Can they be multiple slashes or single-only? 
> > What about trailing slashes - none, single-only, multiple ok?
> > 
> > I start thinking that sentry store should be taking hdfs paths with 
> > "bad" slashes and canoinicalize them internally. I like the idea of a 
> > contract between Sentry components in regards to hdfs paths; we keep 
> > talking about, but the truth is contracts can be inadvertantly broken by 
> > code changes. Canonicalizing paths seems like a cheap operation, so I think 
> > that Sentry store should be fine accepting non-canonicalized paths - in 
> > fact, we should do just that and test the resuts by passing 
> > non-canonicalized paths and make sure canonicalization happened.
> > 
> > Would like others to weigh in.
> 
> Vadim Spector wrote:
> No matter what we decide, should probably document those assuimptions in 
> the test code, if only in couple of sentences.

Vadim, the newer version doesn't have the specific line you have commented 
against. In the new version all the test cases are unchanged with respect to 
the inputs that are being fed


- Arjun


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


On Nov. 27, 2017, 6:03 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63596/
> ---
> 
> (Updated Nov. 27, 2017, 6:03 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, Na 
> Li, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The old retrieveFullPathsImage() method in SentryStore is no longer used by 
> actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
> instead. It was preserved because it is used by test which now doesn't make 
> much sense.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  b355630e7 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  f09d1b228 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  24b11f724 
> 
> 
> Diff: https://reviews.apache.org/r/63596/diff/2/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestSentryHDFSServiceProcessor
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestImageRetriever.java
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-28 Thread Vadim Spector via Review Board 

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




sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Line 2514 (original), 2513 (patched)


So, what are our final assumptions about leading and trailing hashes in 
hdfs paths at the point of adding them to Sentry store? Do we assume they only 
have leading slashes? Can they be multiple slashes or single-only? What about 
trailing slashes - none, single-only, multiple ok?

I start thinking that sentry store should be taking hdfs paths with "bad" 
slashes and canoinicalize them internally. I like the idea of a contract 
between Sentry components in regards to hdfs paths; we keep talking about, but 
the truth is contracts can be inadvertantly broken by code changes. 
Canonicalizing paths seems like a cheap operation, so I think that Sentry store 
should be fine accepting non-canonicalized paths - in fact, we should do just 
that and test the resuts by passing non-canonicalized paths and make sure 
canonicalization happened.

Would like others to weigh in.


- Vadim Spector


On Nov. 27, 2017, 6:03 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63596/
> ---
> 
> (Updated Nov. 27, 2017, 6:03 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, Na 
> Li, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The old retrieveFullPathsImage() method in SentryStore is no longer used by 
> actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
> instead. It was preserved because it is used by test which now doesn't make 
> much sense.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  b355630e7 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  f09d1b228 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  24b11f724 
> 
> 
> Diff: https://reviews.apache.org/r/63596/diff/2/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestSentryHDFSServiceProcessor
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestImageRetriever.java
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-28 Thread Vadim Spector via Review Board 


> On Nov. 28, 2017, 10:16 p.m., Vadim Spector wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Line 2514 (original), 2513 (patched)
> > 
> >
> > So, what are our final assumptions about leading and trailing hashes in 
> > hdfs paths at the point of adding them to Sentry store? Do we assume they 
> > only have leading slashes? Can they be multiple slashes or single-only? 
> > What about trailing slashes - none, single-only, multiple ok?
> > 
> > I start thinking that sentry store should be taking hdfs paths with 
> > "bad" slashes and canoinicalize them internally. I like the idea of a 
> > contract between Sentry components in regards to hdfs paths; we keep 
> > talking about, but the truth is contracts can be inadvertantly broken by 
> > code changes. Canonicalizing paths seems like a cheap operation, so I think 
> > that Sentry store should be fine accepting non-canonicalized paths - in 
> > fact, we should do just that and test the resuts by passing 
> > non-canonicalized paths and make sure canonicalization happened.
> > 
> > Would like others to weigh in.

No matter what we decide, should probably document those assuimptions in the 
test code, if only in couple of sentences.


- Vadim


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


On Nov. 27, 2017, 6:03 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63596/
> ---
> 
> (Updated Nov. 27, 2017, 6:03 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, Na 
> Li, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The old retrieveFullPathsImage() method in SentryStore is no longer used by 
> actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
> instead. It was preserved because it is used by test which now doesn't make 
> much sense.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  b355630e7 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  f09d1b228 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  24b11f724 
> 
> 
> Diff: https://reviews.apache.org/r/63596/diff/2/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestSentryHDFSServiceProcessor
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestImageRetriever.java
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-27 Thread Arjun Mishra via Review Board 

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

(Updated Nov. 27, 2017, 6 p.m.)


Review request for sentry, Alexander Kolbasov, Sergio Pena, and Vadim Spector.


Changes
---

Updateed patch after SENTRY-2032 was committed


Repository: sentry


Description
---

The old retrieveFullPathsImage() method in SentryStore is no longer used by 
actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
instead. It was preserved because it is used by test which now doesn't make 
much sense.


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
 b355630e7 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
 f09d1b228 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 f32a745ed 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 24b11f724 


Diff: https://reviews.apache.org/r/63596/diff/2/

Changes: https://reviews.apache.org/r/63596/diff/1-2/


Testing
---

mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
-Dtest=TestSentryHDFSServiceProcessor
mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
-Dtest=TestImageRetriever.java


Thanks,

Arjun Mishra

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-06 Thread Arjun Mishra via Review Board 


> On Nov. 6, 2017, 10:11 p.m., Vadim Spector wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 2453 (patched)
> > 
> >
> > In the original test code all paths were absolute, i.e starting with 
> > "/". After your changes, all table paths start with "prefix", which does 
> > _not_ begin with "/". Why is that? What has changed? If it's correct, it's 
> > worth at least one sentence of javadoc.
> 
> Arjun Mishra wrote:
> Yes so based don my testing it is required that the front slash be left 
> out when you call persist full paths image. Please see 
> SentryStore#retrieveFullPathsImageCore, look at line "String[] pathComponents 
> = PathUtils.splitPath(path);". If the starting "/" is included, one of the 
> path elements would be empty and this would affect the tree structure. 
> 
> That's why all "/" have been removed from paths, that are being passed in 
> as arguments into persisting paths image.
> 
> Arjun Mishra wrote:
> All leading "/" have been removed
> 
> Vadim Spector wrote:
> So, again, what has changed? Did the tests passed before? Why did they, 
> if they had leading slashes?
> 
> Arjun Mishra wrote:
> retrieveFullPathsImage(), and 
> retrieveFullPathsImageCore(PersistenceManager pm, long currentSnapshotID) 
> methods have been removed and replaced by retrieveFullPathsImageUpdate(final 
> String[] prefixes), retrieveFullPathsImageCore(PersistenceManager pm, long 
> currentSnapshotID, UpdateableAuthzPaths pathUpdate). These changes were 
> because SENTRY-1915 had made those methods obsolete, but the tst cases 
> continued to reference them.

The difference is because retrieveFullPathsImageCore is now splitting each path 
string and converting to list of path components by calling 
PathsUpdate.applyAddChanges. We never did this before. Before we were simply 
returning all path strings. 


The below code fragement (not in old code) is responsible for need to not have 
leading "/" slashes. See retrieveFullPathsImageCore(PersistenceManager pm, long 
currentSnapshotID, UpdateableAuthzPaths pathUpdate)

for (MAuthzPathsMapping authzToPaths : authzToPathsMappings) {
  String  objName = authzToPaths.getAuthzObjName();
  // Convert path strings to list of components
  for (String path: authzToPaths.getPathStrings()) {
String[] pathComponents = PathUtils.splitPath(path);
List paths = new ArrayList<>(pathComponents.length);
Collections.addAll(paths, pathComponents);
pathUpdate.applyAddChanges(objName, Collections.singletonList(paths));
  }
}


- Arjun


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


On Nov. 6, 2017, 9:52 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63596/
> ---
> 
> (Updated Nov. 6, 2017, 9:52 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The old retrieveFullPathsImage() method in SentryStore is no longer used by 
> actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
> instead. It was preserved because it is used by test which now doesn't make 
> much sense.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  b355630e7 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  f09d1b228 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  0cd6e48aa 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  cf83e7796 
> 
> 
> Diff: https://reviews.apache.org/r/63596/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestSentryHDFSServiceProcessor
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestImageRetriever.java
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-06 Thread Arjun Mishra via Review Board 


> On Nov. 6, 2017, 10:11 p.m., Vadim Spector wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 2453 (patched)
> > 
> >
> > In the original test code all paths were absolute, i.e starting with 
> > "/". After your changes, all table paths start with "prefix", which does 
> > _not_ begin with "/". Why is that? What has changed? If it's correct, it's 
> > worth at least one sentence of javadoc.
> 
> Arjun Mishra wrote:
> Yes so based don my testing it is required that the front slash be left 
> out when you call persist full paths image. Please see 
> SentryStore#retrieveFullPathsImageCore, look at line "String[] pathComponents 
> = PathUtils.splitPath(path);". If the starting "/" is included, one of the 
> path elements would be empty and this would affect the tree structure. 
> 
> That's why all "/" have been removed from paths, that are being passed in 
> as arguments into persisting paths image.
> 
> Arjun Mishra wrote:
> All leading "/" have been removed
> 
> Vadim Spector wrote:
> So, again, what has changed? Did the tests passed before? Why did they, 
> if they had leading slashes?

retrieveFullPathsImage(), and retrieveFullPathsImageCore(PersistenceManager pm, 
long currentSnapshotID) methods have been removed and replaced by 
retrieveFullPathsImageUpdate(final String[] prefixes), 
retrieveFullPathsImageCore(PersistenceManager pm, long currentSnapshotID, 
UpdateableAuthzPaths pathUpdate). These changes were because SENTRY-1915 had 
made those methods obsolete, but the tst cases continued to reference them.


- Arjun


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


On Nov. 6, 2017, 9:52 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63596/
> ---
> 
> (Updated Nov. 6, 2017, 9:52 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The old retrieveFullPathsImage() method in SentryStore is no longer used by 
> actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
> instead. It was preserved because it is used by test which now doesn't make 
> much sense.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  b355630e7 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  f09d1b228 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  0cd6e48aa 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  cf83e7796 
> 
> 
> Diff: https://reviews.apache.org/r/63596/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestSentryHDFSServiceProcessor
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestImageRetriever.java
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-06 Thread Vadim Spector via Review Board 


> On Nov. 6, 2017, 10:11 p.m., Vadim Spector wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 2453 (patched)
> > 
> >
> > In the original test code all paths were absolute, i.e starting with 
> > "/". After your changes, all table paths start with "prefix", which does 
> > _not_ begin with "/". Why is that? What has changed? If it's correct, it's 
> > worth at least one sentence of javadoc.
> 
> Arjun Mishra wrote:
> Yes so based don my testing it is required that the front slash be left 
> out when you call persist full paths image. Please see 
> SentryStore#retrieveFullPathsImageCore, look at line "String[] pathComponents 
> = PathUtils.splitPath(path);". If the starting "/" is included, one of the 
> path elements would be empty and this would affect the tree structure. 
> 
> That's why all "/" have been removed from paths, that are being passed in 
> as arguments into persisting paths image.
> 
> Arjun Mishra wrote:
> All leading "/" have been removed

So, again, what has changed? Did the tests passed before? Why did they, if they 
had leading slashes?


- Vadim


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


On Nov. 6, 2017, 9:52 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63596/
> ---
> 
> (Updated Nov. 6, 2017, 9:52 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The old retrieveFullPathsImage() method in SentryStore is no longer used by 
> actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
> instead. It was preserved because it is used by test which now doesn't make 
> much sense.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  b355630e7 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  f09d1b228 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  0cd6e48aa 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  cf83e7796 
> 
> 
> Diff: https://reviews.apache.org/r/63596/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestSentryHDFSServiceProcessor
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestImageRetriever.java
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-06 Thread Arjun Mishra via Review Board 


> On Nov. 6, 2017, 10:11 p.m., Vadim Spector wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 2453 (patched)
> > 
> >
> > In the original test code all paths were absolute, i.e starting with 
> > "/". After your changes, all table paths start with "prefix", which does 
> > _not_ begin with "/". Why is that? What has changed? If it's correct, it's 
> > worth at least one sentence of javadoc.
> 
> Arjun Mishra wrote:
> Yes so based don my testing it is required that the front slash be left 
> out when you call persist full paths image. Please see 
> SentryStore#retrieveFullPathsImageCore, look at line "String[] pathComponents 
> = PathUtils.splitPath(path);". If the starting "/" is included, one of the 
> path elements would be empty and this would affect the tree structure. 
> 
> That's why all "/" have been removed from paths, that are being passed in 
> as arguments into persisting paths image.

All leading "/" have been removed


- Arjun


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


On Nov. 6, 2017, 9:52 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63596/
> ---
> 
> (Updated Nov. 6, 2017, 9:52 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The old retrieveFullPathsImage() method in SentryStore is no longer used by 
> actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
> instead. It was preserved because it is used by test which now doesn't make 
> much sense.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  b355630e7 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  f09d1b228 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  0cd6e48aa 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  cf83e7796 
> 
> 
> Diff: https://reviews.apache.org/r/63596/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestSentryHDFSServiceProcessor
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestImageRetriever.java
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-06 Thread Arjun Mishra via Review Board 


> On Nov. 6, 2017, 10:11 p.m., Vadim Spector wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 2453 (patched)
> > 
> >
> > In the original test code all paths were absolute, i.e starting with 
> > "/". After your changes, all table paths start with "prefix", which does 
> > _not_ begin with "/". Why is that? What has changed? If it's correct, it's 
> > worth at least one sentence of javadoc.

Yes so based don my testing it is required that the front slash be left out 
when you call persist full paths image. Please see 
SentryStore#retrieveFullPathsImageCore, look at line "String[] pathComponents = 
PathUtils.splitPath(path);". If the starting "/" is included, one of the path 
elements would be empty and this would affect the tree structure. 

That's why all "/" have been removed from paths, that are being passed in as 
arguments into persisting paths image.


- Arjun


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


On Nov. 6, 2017, 9:52 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63596/
> ---
> 
> (Updated Nov. 6, 2017, 9:52 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The old retrieveFullPathsImage() method in SentryStore is no longer used by 
> actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
> instead. It was preserved because it is used by test which now doesn't make 
> much sense.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  b355630e7 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  f09d1b228 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  0cd6e48aa 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  cf83e7796 
> 
> 
> Diff: https://reviews.apache.org/r/63596/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestSentryHDFSServiceProcessor
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestImageRetriever.java
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-06 Thread Vadim Spector via Review Board 

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




sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 2453 (patched)


In the original test code all paths were absolute, i.e starting with "/". 
After your changes, all table paths start with "prefix", which does _not_ begin 
with "/". Why is that? What has changed? If it's correct, it's worth at least 
one sentence of javadoc.


- Vadim Spector


On Nov. 6, 2017, 9:52 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63596/
> ---
> 
> (Updated Nov. 6, 2017, 9:52 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The old retrieveFullPathsImage() method in SentryStore is no longer used by 
> actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
> instead. It was preserved because it is used by test which now doesn't make 
> much sense.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  b355630e7 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  f09d1b228 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  0cd6e48aa 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  cf83e7796 
> 
> 
> Diff: https://reviews.apache.org/r/63596/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestSentryHDFSServiceProcessor
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
> -Dtest=TestImageRetriever.java
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-06 Thread Arjun Mishra via Review Board 

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

Review request for sentry, Alexander Kolbasov, Sergio Pena, and Vadim Spector.


Repository: sentry


Description
---

The old retrieveFullPathsImage() method in SentryStore is no longer used by 
actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used 
instead. It was preserved because it is used by test which now doesn't make 
much sense.


Diffs
-

  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
 b355630e7 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
 f09d1b228 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 0cd6e48aa 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 cf83e7796 


Diff: https://reviews.apache.org/r/63596/diff/1/


Testing
---

mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
-Dtest=TestSentryHDFSServiceProcessor
mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test 
-Dtest=TestImageRetriever.java


Thanks,

Arjun Mishra