Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider should be covered with unit tests

2016-09-22 Thread Chaoyu Tang 

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


Ship it!




LGTM. Thanks Illya!.

- Chaoyu Tang


On Sept. 22, 2016, 5:46 p.m., Illya Yalovyy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51694/
> ---
> 
> (Updated Sept. 22, 2016, 5:46 p.m.)
> 
> 
> Review request for hive, Ashutosh Chauhan, Chaoyu Tang, Naveen Gangam, and 
> Szehon Ho.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Currently LdapAuthenticationProviderImpl class is not covered with unit 
> tests. To make this class testable some minor refactoring will be required.
> 
> 
> Diffs
> -
> 
>   service/pom.xml ecea719 
>   
> service/src/java/org/apache/hive/service/auth/LdapAuthenticationProviderImpl.java
>  efd5393 
>   service/src/java/org/apache/hive/service/auth/ldap/ChainFilterFactory.java 
> PRE-CREATION 
>   
> service/src/java/org/apache/hive/service/auth/ldap/CustomQueryFilterFactory.java
>  PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearch.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearchFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/Filter.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/FilterFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearchFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/Query.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/QueryFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/SearchResultHandler.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/UserFilterFactory.java 
> PRE-CREATION 
>   
> service/src/java/org/apache/hive/service/auth/ldap/UserSearchFilterFactory.java
>  PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/TestLdapAtnProviderWithMiniDS.java
>  089a059 
>   
> service/src/test/org/apache/hive/service/auth/TestLdapAuthenticationProviderImpl.java
>  f276906 
>   service/src/test/org/apache/hive/service/auth/ldap/Credentials.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/LdapTestUtils.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestChainFilter.java 
> PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/ldap/TestCustomQueryFilter.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestGroupFilter.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestLdapSearch.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestLdapUtils.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestQuery.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestQueryFactory.java 
> PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/ldap/TestSearchResultHandler.java
>  PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestUserFilter.java 
> PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/ldap/TestUserSearchFilter.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51694/diff/
> 
> 
> Testing
> ---
> 
> ...hive/service> mvn clean test
> 
> ...
> 
> Results :
> 
> Tests run: 123, Failures: 0, Errors: 0, Skipped: 0
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 04:18 min
> [INFO] Finished at: 2016-09-06T08:46:04-07:00
> [INFO] Final Memory: 66M/984M
> [INFO] 
> 
> 
> 
> Thanks,
> 
> Illya Yalovyy
> 
>

Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider should be covered with unit tests

2016-09-22 Thread Illya Yalovyy 

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

(Updated Sept. 22, 2016, 5:46 p.m.)


Review request for hive, Ashutosh Chauhan, Chaoyu Tang, Naveen Gangam, and 
Szehon Ho.


Changes
---

Use "limit 2" for LDAP search queries that are expected to return a single 
result.


Repository: hive-git


Description
---

Currently LdapAuthenticationProviderImpl class is not covered with unit tests. 
To make this class testable some minor refactoring will be required.


Diffs (updated)
-

  service/pom.xml ecea719 
  
service/src/java/org/apache/hive/service/auth/LdapAuthenticationProviderImpl.java
 efd5393 
  service/src/java/org/apache/hive/service/auth/ldap/ChainFilterFactory.java 
PRE-CREATION 
  
service/src/java/org/apache/hive/service/auth/ldap/CustomQueryFilterFactory.java
 PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/DirSearch.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/DirSearchFactory.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/Filter.java PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/FilterFactory.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/LdapSearchFactory.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/Query.java PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/QueryFactory.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/SearchResultHandler.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/UserFilterFactory.java 
PRE-CREATION 
  
service/src/java/org/apache/hive/service/auth/ldap/UserSearchFilterFactory.java 
PRE-CREATION 
  
service/src/test/org/apache/hive/service/auth/TestLdapAtnProviderWithMiniDS.java
 089a059 
  
service/src/test/org/apache/hive/service/auth/TestLdapAuthenticationProviderImpl.java
 f276906 
  service/src/test/org/apache/hive/service/auth/ldap/Credentials.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/LdapTestUtils.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/TestChainFilter.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/TestCustomQueryFilter.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/TestGroupFilter.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/TestLdapSearch.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/TestLdapUtils.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/TestQuery.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/TestQueryFactory.java 
PRE-CREATION 
  
service/src/test/org/apache/hive/service/auth/ldap/TestSearchResultHandler.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/TestUserFilter.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/TestUserSearchFilter.java 
PRE-CREATION 

Diff: https://reviews.apache.org/r/51694/diff/


Testing
---

...hive/service> mvn clean test

...

Results :

Tests run: 123, Failures: 0, Errors: 0, Skipped: 0

[INFO] 
[INFO] BUILD SUCCESS
[INFO] 
[INFO] Total time: 04:18 min
[INFO] Finished at: 2016-09-06T08:46:04-07:00
[INFO] Final Memory: 66M/984M
[INFO] 


Thanks,

Illya Yalovyy

Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider should be covered with unit tests

2016-09-21 Thread Illya Yalovyy 


> On Sept. 21, 2016, 12:50 a.m., Szehon Ho wrote:
> > This looks like a great refactoring to me. This class was always hard to 
> > understand, and this makes it a little easier.  I'll defer to Chaoyu to the 
> > comments.

Thank you!


- Illya


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


On Sept. 20, 2016, 7:39 p.m., Illya Yalovyy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51694/
> ---
> 
> (Updated Sept. 20, 2016, 7:39 p.m.)
> 
> 
> Review request for hive, Ashutosh Chauhan, Chaoyu Tang, Naveen Gangam, and 
> Szehon Ho.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Currently LdapAuthenticationProviderImpl class is not covered with unit 
> tests. To make this class testable some minor refactoring will be required.
> 
> 
> Diffs
> -
> 
>   service/pom.xml ecea719 
>   
> service/src/java/org/apache/hive/service/auth/LdapAuthenticationProviderImpl.java
>  efd5393 
>   service/src/java/org/apache/hive/service/auth/ldap/ChainFilterFactory.java 
> PRE-CREATION 
>   
> service/src/java/org/apache/hive/service/auth/ldap/CustomQueryFilterFactory.java
>  PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearch.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearchFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/Filter.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/FilterFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearchFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/Query.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/QueryFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/SearchResultHandler.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/UserFilterFactory.java 
> PRE-CREATION 
>   
> service/src/java/org/apache/hive/service/auth/ldap/UserSearchFilterFactory.java
>  PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/TestLdapAtnProviderWithMiniDS.java
>  089a059 
>   
> service/src/test/org/apache/hive/service/auth/TestLdapAuthenticationProviderImpl.java
>  f276906 
>   service/src/test/org/apache/hive/service/auth/ldap/Credentials.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/LdapTestUtils.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestChainFilter.java 
> PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/ldap/TestCustomQueryFilter.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestGroupFilter.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestLdapSearch.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestLdapUtils.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestQuery.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestQueryFactory.java 
> PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/ldap/TestSearchResultHandler.java
>  PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestUserFilter.java 
> PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/ldap/TestUserSearchFilter.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51694/diff/
> 
> 
> Testing
> ---
> 
> ...hive/service> mvn clean test
> 
> ...
> 
> Results :
> 
> Tests run: 123, Failures: 0, Errors: 0, Skipped: 0
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 04:18 min
> [INFO] Finished at: 2016-09-06T08:46:04-07:00
> [INFO] Final Memory: 66M/984M
> [INFO] 
> 
> 
> 
> Thanks,
> 
> Illya Yalovyy
> 
>

Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider should be covered with unit tests

2016-09-21 Thread Illya Yalovyy 


> On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/Query.java, line 122
> > 
> >
> > Will it improve the performance to set the search limit? I did not see 
> > it is used.
> 
> Illya Yalovyy wrote:
> I will be used for different filters. Do you think we should use it for 
> existing filters? Which one in particular? Or you would prefer me to remove 
> this option?
> 
> Please keep in mind that this CR is not about performance.
> 
> Chaoyu Tang wrote:
> I thought in the existing implementation, the search limits in some 
> methods like findGroupDNByName, findUserDNByName are set to 2 to reduce the 
> returned results in case there are many, is not it?

It does make sense. I'll make this change.


- Illya


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


On Sept. 20, 2016, 7:39 p.m., Illya Yalovyy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51694/
> ---
> 
> (Updated Sept. 20, 2016, 7:39 p.m.)
> 
> 
> Review request for hive, Ashutosh Chauhan, Chaoyu Tang, Naveen Gangam, and 
> Szehon Ho.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Currently LdapAuthenticationProviderImpl class is not covered with unit 
> tests. To make this class testable some minor refactoring will be required.
> 
> 
> Diffs
> -
> 
>   service/pom.xml ecea719 
>   
> service/src/java/org/apache/hive/service/auth/LdapAuthenticationProviderImpl.java
>  efd5393 
>   service/src/java/org/apache/hive/service/auth/ldap/ChainFilterFactory.java 
> PRE-CREATION 
>   
> service/src/java/org/apache/hive/service/auth/ldap/CustomQueryFilterFactory.java
>  PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearch.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearchFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/Filter.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/FilterFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearchFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/Query.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/QueryFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/SearchResultHandler.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/UserFilterFactory.java 
> PRE-CREATION 
>   
> service/src/java/org/apache/hive/service/auth/ldap/UserSearchFilterFactory.java
>  PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/TestLdapAtnProviderWithMiniDS.java
>  089a059 
>   
> service/src/test/org/apache/hive/service/auth/TestLdapAuthenticationProviderImpl.java
>  f276906 
>   service/src/test/org/apache/hive/service/auth/ldap/Credentials.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/LdapTestUtils.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestChainFilter.java 
> PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/ldap/TestCustomQueryFilter.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestGroupFilter.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestLdapSearch.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestLdapUtils.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestQuery.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestQueryFactory.java 
> PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/ldap/TestSearchResultHandler.java
>  PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestUserFilter.java 
> PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/ldap/TestUserSearchFilter.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51694/diff/
> 
> 
> Testing
> ---
> 
> ...hive/service> mvn clean test
> 
> ...
> 
> Results :
> 
> Tests run: 123, Failures: 0, Errors: 0, Skipped: 0
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
>

Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider should be covered with unit tests

2016-09-20 Thread Chaoyu Tang 


> On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/Query.java, line 122
> > 
> >
> > Will it improve the performance to set the search limit? I did not see 
> > it is used.
> 
> Illya Yalovyy wrote:
> I will be used for different filters. Do you think we should use it for 
> existing filters? Which one in particular? Or you would prefer me to remove 
> this option?
> 
> Please keep in mind that this CR is not about performance.

I thought in the existing implementation, the search limits in some methods 
like findGroupDNByName, findUserDNByName are set to 2 to reduce the returned 
results in case there are many, is not it?


- Chaoyu


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


On Sept. 20, 2016, 7:39 p.m., Illya Yalovyy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51694/
> ---
> 
> (Updated Sept. 20, 2016, 7:39 p.m.)
> 
> 
> Review request for hive, Ashutosh Chauhan, Chaoyu Tang, Naveen Gangam, and 
> Szehon Ho.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Currently LdapAuthenticationProviderImpl class is not covered with unit 
> tests. To make this class testable some minor refactoring will be required.
> 
> 
> Diffs
> -
> 
>   service/pom.xml ecea719 
>   
> service/src/java/org/apache/hive/service/auth/LdapAuthenticationProviderImpl.java
>  efd5393 
>   service/src/java/org/apache/hive/service/auth/ldap/ChainFilterFactory.java 
> PRE-CREATION 
>   
> service/src/java/org/apache/hive/service/auth/ldap/CustomQueryFilterFactory.java
>  PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearch.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearchFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/Filter.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/FilterFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearchFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/Query.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/QueryFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/SearchResultHandler.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/UserFilterFactory.java 
> PRE-CREATION 
>   
> service/src/java/org/apache/hive/service/auth/ldap/UserSearchFilterFactory.java
>  PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/TestLdapAtnProviderWithMiniDS.java
>  089a059 
>   
> service/src/test/org/apache/hive/service/auth/TestLdapAuthenticationProviderImpl.java
>  f276906 
>   service/src/test/org/apache/hive/service/auth/ldap/Credentials.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/LdapTestUtils.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestChainFilter.java 
> PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/ldap/TestCustomQueryFilter.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestGroupFilter.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestLdapSearch.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestLdapUtils.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestQuery.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestQueryFactory.java 
> PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/ldap/TestSearchResultHandler.java
>  PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestUserFilter.java 
> PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/ldap/TestUserSearchFilter.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51694/diff/
> 
> 
> Testing
> ---
> 
> ...hive/service> mvn clean test
> 
> ...
> 
> Results :
> 
> Tests run: 123, Failures: 0, Errors: 0, Skipped: 0
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 04:18 min
> [INFO] Finished at:

Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider should be covered with unit tests

2016-09-20 Thread Chaoyu Tang 


> On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java, line 105
> > 
> >
> > This method might throw out runtime exception such as NPE, 
> > IndexOutOfBoundsException, should we check the passed in parameter rdn? 
> > We might not run into this situation in old code, but since this line 
> > of code is refactored as a separate API, I think we should do the check. 
> > Same for the other methods like patternToBaseDn etc.
> 
> Illya Yalovyy wrote:
> Agree. The DN parsing in general implemented quite poorly. I have a task 
> already to re-implement it completely. There are many problems with current 
> one. Handling incorrect format or invalid input is only one of them. My 
> intention is to use RDN java class to do a correct parsing. 
> 
> https://docs.oracle.com/javase/7/docs/api/javax/naming/ldap/Rdn.html
> 
> I think we can leave it for now, and I will submit another CR that 
> addresses this concern sortly.

OK


> On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java, line 108
> > 
> >
> > can getSingleLdapName be used to enforce only one returned entry? that 
> > API in SearachResultHandler is never used.
> 
> Illya Yalovyy wrote:
> I was trying to copy existing logic. At the moment, I don't want to do 
> any changes to that. This particular code should be improved in separate CR. 
> Does it make sense?

OK


> On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java, line 159
> > 
> >
> > I am not sure if there is any precedence for these configurations, but 
> > here it seems that the GUIDKEY/BASEDN takes precedence over DNPATTERN, 
> > which is different from the existing implementation and cause the behavior 
> > change.
> 
> Illya Yalovyy wrote:
> Could you please give more details on the case when the behavior will be 
> different. 
> The logic seems to be same: It uses GUIDKEY/BASEDN only when DNPATTERN is 
> not configured:
> 
> if (StringUtils.isBlank(patternsString)) {
> ...
> 
> Which means *only* if patternsString is blank, try to use GUIDKEY/BASEDN.
> 
> Please let me know if I did not get it correctly.

Never mind.


- Chaoyu


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


On Sept. 20, 2016, 7:39 p.m., Illya Yalovyy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51694/
> ---
> 
> (Updated Sept. 20, 2016, 7:39 p.m.)
> 
> 
> Review request for hive, Ashutosh Chauhan, Chaoyu Tang, Naveen Gangam, and 
> Szehon Ho.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Currently LdapAuthenticationProviderImpl class is not covered with unit 
> tests. To make this class testable some minor refactoring will be required.
> 
> 
> Diffs
> -
> 
>   service/pom.xml ecea719 
>   
> service/src/java/org/apache/hive/service/auth/LdapAuthenticationProviderImpl.java
>  efd5393 
>   service/src/java/org/apache/hive/service/auth/ldap/ChainFilterFactory.java 
> PRE-CREATION 
>   
> service/src/java/org/apache/hive/service/auth/ldap/CustomQueryFilterFactory.java
>  PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearch.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearchFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/Filter.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/FilterFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearchFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/Query.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/QueryFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/SearchResultHandler.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/UserFilterFactory.java 
> PRE-CREATION 
>   
> service/src/java/org/apache/hive/service/auth/ldap/UserSearchFilterFactory.java
>  PRE-CREATION 
>   
>

Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider should be covered with unit tests

2016-09-20 Thread Szehon Ho 

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


Ship it!




This looks like a great refactoring to me. This class was always hard to 
understand, and this makes it a little easier.  I'll defer to Chaoyu to the 
comments.

- Szehon Ho


On Sept. 20, 2016, 7:39 p.m., Illya Yalovyy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51694/
> ---
> 
> (Updated Sept. 20, 2016, 7:39 p.m.)
> 
> 
> Review request for hive, Ashutosh Chauhan, Chaoyu Tang, Naveen Gangam, and 
> Szehon Ho.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Currently LdapAuthenticationProviderImpl class is not covered with unit 
> tests. To make this class testable some minor refactoring will be required.
> 
> 
> Diffs
> -
> 
>   service/pom.xml ecea719 
>   
> service/src/java/org/apache/hive/service/auth/LdapAuthenticationProviderImpl.java
>  efd5393 
>   service/src/java/org/apache/hive/service/auth/ldap/ChainFilterFactory.java 
> PRE-CREATION 
>   
> service/src/java/org/apache/hive/service/auth/ldap/CustomQueryFilterFactory.java
>  PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearch.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearchFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/Filter.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/FilterFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearchFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/Query.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/QueryFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/SearchResultHandler.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/UserFilterFactory.java 
> PRE-CREATION 
>   
> service/src/java/org/apache/hive/service/auth/ldap/UserSearchFilterFactory.java
>  PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/TestLdapAtnProviderWithMiniDS.java
>  089a059 
>   
> service/src/test/org/apache/hive/service/auth/TestLdapAuthenticationProviderImpl.java
>  f276906 
>   service/src/test/org/apache/hive/service/auth/ldap/Credentials.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/LdapTestUtils.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestChainFilter.java 
> PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/ldap/TestCustomQueryFilter.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestGroupFilter.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestLdapSearch.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestLdapUtils.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestQuery.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestQueryFactory.java 
> PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/ldap/TestSearchResultHandler.java
>  PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestUserFilter.java 
> PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/ldap/TestUserSearchFilter.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51694/diff/
> 
> 
> Testing
> ---
> 
> ...hive/service> mvn clean test
> 
> ...
> 
> Results :
> 
> Tests run: 123, Failures: 0, Errors: 0, Skipped: 0
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 04:18 min
> [INFO] Finished at: 2016-09-06T08:46:04-07:00
> [INFO] Final Memory: 66M/984M
> [INFO] 
> 
> 
> 
> Thanks,
> 
> Illya Yalovyy
> 
>

Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider should be covered with unit tests

2016-09-20 Thread Illya Yalovyy 


> On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java, 
> > line 37
> > 
> >
> > Do we really need an extra factory layer and have a factory for each 
> > filter?
> > In Hive, actaully each session instantiates its own 
> > LdapAuthenticationProviderImpl, which now contains different factories with 
> > each one generating only one instance of its filter.

I think so. Factories encapsulate logic related to choosing whether particular 
filter is required or not based on the provided configuration.


> On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java, line 108
> > 
> >
> > can getSingleLdapName be used to enforce only one returned entry? that 
> > API in SearachResultHandler is never used.

I was trying to copy existing logic. At the moment, I don't want to do any 
changes to that. This particular code should be improved in separate CR. Does 
it make sense?


> On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java, line 105
> > 
> >
> > This method might throw out runtime exception such as NPE, 
> > IndexOutOfBoundsException, should we check the passed in parameter rdn? 
> > We might not run into this situation in old code, but since this line 
> > of code is refactored as a separate API, I think we should do the check. 
> > Same for the other methods like patternToBaseDn etc.

Agree. The DN parsing in general implemented quite poorly. I have a task 
already to re-implement it completely. There are many problems with current 
one. Handling incorrect format or invalid input is only one of them. My 
intention is to use RDN java class to do a correct parsing. 

https://docs.oracle.com/javase/7/docs/api/javax/naming/ldap/Rdn.html

I think we can leave it for now, and I will submit another CR that addresses 
this concern sortly.


> On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java, line 159
> > 
> >
> > I am not sure if there is any precedence for these configurations, but 
> > here it seems that the GUIDKEY/BASEDN takes precedence over DNPATTERN, 
> > which is different from the existing implementation and cause the behavior 
> > change.

Could you please give more details on the case when the behavior will be 
different. 
The logic seems to be same: It uses GUIDKEY/BASEDN only when DNPATTERN is not 
configured:

if (StringUtils.isBlank(patternsString)) {
...

Which means *only* if patternsString is blank, try to use GUIDKEY/BASEDN.

Please let me know if I did not get it correctly.


> On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/Query.java, line 122
> > 
> >
> > Will it improve the performance to set the search limit? I did not see 
> > it is used.

I will be used for different filters. Do you think we should use it for 
existing filters? Which one in particular? Or you would prefer me to remove 
this option?

Please keep in mind that this CR is not about performance.


- Illya


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


On Sept. 7, 2016, 2:24 p.m., Illya Yalovyy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51694/
> ---
> 
> (Updated Sept. 7, 2016, 2:24 p.m.)
> 
> 
> Review request for hive, Ashutosh Chauhan, Chaoyu Tang, Naveen Gangam, and 
> Szehon Ho.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Currently LdapAuthenticationProviderImpl class is not covered with unit 
> tests. To make this class testable some minor refactoring will be required.
> 
> 
> Diffs
> -
> 
>   service/pom.xml ecea719 
>   
> service/src/java/org/apache/hive/service/auth/LdapAuthenticationProviderImpl.java
>  efd5393 
>   service/src/java/org/apache/hive/service/auth/ldap/ChainFilterFactory.java 
> PRE-CREATION 
>   
> service/src/java/org/apache/hive/service/auth/ldap/CustomQueryFilterFactory.java
>  PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearch.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearchFactory.java 
> PRE-CREATION 
>

Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider should be covered with unit tests

2016-09-19 Thread Illya Yalovyy 

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



Chaoyu,

Thank you for a great review. I have also done some minor changes on my side. 
I'll update this CR in a couple of days.

- Illya Yalovyy


On Sept. 7, 2016, 2:24 p.m., Illya Yalovyy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51694/
> ---
> 
> (Updated Sept. 7, 2016, 2:24 p.m.)
> 
> 
> Review request for hive, Ashutosh Chauhan, Chaoyu Tang, Naveen Gangam, and 
> Szehon Ho.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Currently LdapAuthenticationProviderImpl class is not covered with unit 
> tests. To make this class testable some minor refactoring will be required.
> 
> 
> Diffs
> -
> 
>   service/pom.xml ecea719 
>   
> service/src/java/org/apache/hive/service/auth/LdapAuthenticationProviderImpl.java
>  efd5393 
>   service/src/java/org/apache/hive/service/auth/ldap/ChainFilterFactory.java 
> PRE-CREATION 
>   
> service/src/java/org/apache/hive/service/auth/ldap/CustomQueryFilterFactory.java
>  PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearch.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearchFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/Filter.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/FilterFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearchFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/Query.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/QueryFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/SearchResultHandler.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/UserFilterFactory.java 
> PRE-CREATION 
>   
> service/src/java/org/apache/hive/service/auth/ldap/UserSearchFilterFactory.java
>  PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/TestLdapAtnProviderWithMiniDS.java
>  089a059 
>   
> service/src/test/org/apache/hive/service/auth/TestLdapAuthenticationProviderImpl.java
>  f276906 
>   service/src/test/org/apache/hive/service/auth/ldap/Credentials.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/LdapTestUtils.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestChainFilter.java 
> PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/ldap/TestCustomQueryFilter.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestGroupFilter.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestLdapSearch.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestLdapUtils.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestQuery.java 
> PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestQueryFactory.java 
> PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/ldap/TestSearchResultHandler.java
>  PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestUserFilter.java 
> PRE-CREATION 
>   
> service/src/test/org/apache/hive/service/auth/ldap/TestUserSearchFilter.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51694/diff/
> 
> 
> Testing
> ---
> 
> ...hive/service> mvn clean test
> 
> ...
> 
> Results :
> 
> Tests run: 123, Failures: 0, Errors: 0, Skipped: 0
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 04:18 min
> [INFO] Finished at: 2016-09-06T08:46:04-07:00
> [INFO] Final Memory: 66M/984M
> [INFO] 
> 
> 
> 
> Thanks,
> 
> Illya Yalovyy
> 
>

Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider should be covered with unit tests

2016-09-16 Thread Chaoyu Tang 

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




service/pom.xml (line 165)


1. Need remove the empty spaces (in red squares)
2. Should we add the version 
${mockito-all.version}. currently the 
${mockito-all.version} in Hive is 1.9.5.



service/src/java/org/apache/hive/service/auth/ldap/ChainFilterFactory.java 
(line 46)


Need remove all the extra empty spaces (in red squares) here and also other 
places.



service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java 
(line 37)


Do we really need an extra factory layer and have a factory for each filter?
In Hive, actaully each session instantiates its own 
LdapAuthenticationProviderImpl, which now contains different factories with 
each one generating only one instance of its filter.



service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java (line 62)


Is not it the LdapUtils.patternsToBaseDns(userPatterns)?



service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java (line 89)


I don't think it is necessary to cache the user/userDn and also it might be 
a potential security issue.



service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java (line 108)


can getSingleLdapName be used to enforce only one returned entry? that API 
in SearachResultHandler is never used.



service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java (line 105)


This method might throw out runtime exception such as NPE, 
IndexOutOfBoundsException, should we check the passed in parameter rdn? 
We might not run into this situation in old code, but since this line of 
code is refactored as a separate API, I think we should do the check. Same for 
the other methods like patternToBaseDn etc.



service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java (line 159)


I am not sure if there is any precedence for these configurations, but here 
it seems that the GUIDKEY/BASEDN takes precedence over DNPATTERN, which is 
different from the existing implementation and cause the behavior change.



service/src/java/org/apache/hive/service/auth/ldap/Query.java (line 122)


Will it improve the performance to set the search limit? I did not see it 
is used.


- Chaoyu Tang


On Sept. 7, 2016, 2:24 p.m., Illya Yalovyy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51694/
> ---
> 
> (Updated Sept. 7, 2016, 2:24 p.m.)
> 
> 
> Review request for hive, Ashutosh Chauhan, Chaoyu Tang, Naveen Gangam, and 
> Szehon Ho.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Currently LdapAuthenticationProviderImpl class is not covered with unit 
> tests. To make this class testable some minor refactoring will be required.
> 
> 
> Diffs
> -
> 
>   service/pom.xml ecea719 
>   
> service/src/java/org/apache/hive/service/auth/LdapAuthenticationProviderImpl.java
>  efd5393 
>   service/src/java/org/apache/hive/service/auth/ldap/ChainFilterFactory.java 
> PRE-CREATION 
>   
> service/src/java/org/apache/hive/service/auth/ldap/CustomQueryFilterFactory.java
>  PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearch.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearchFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/Filter.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/FilterFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearchFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/Query.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/QueryFactory.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/SearchResultHandler.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/UserFilterFactory.java 
> PRE-CREATION 
>   
>

Review Request 51694: HIVE-14713 LDAP Authentication Provider should be covered with unit tests

2016-09-07 Thread Illya Yalovyy 

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

Review request for hive, Ashutosh Chauhan, Chaoyu Tang, Naveen Gangam, and 
Szehon Ho.


Repository: hive-git


Description
---

Currently LdapAuthenticationProviderImpl class is not covered with unit tests. 
To make this class testable some minor refactoring will be required.


Diffs
-

  service/pom.xml ecea719 
  
service/src/java/org/apache/hive/service/auth/LdapAuthenticationProviderImpl.java
 efd5393 
  service/src/java/org/apache/hive/service/auth/ldap/ChainFilterFactory.java 
PRE-CREATION 
  
service/src/java/org/apache/hive/service/auth/ldap/CustomQueryFilterFactory.java
 PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/DirSearch.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/DirSearchFactory.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/Filter.java PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/FilterFactory.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/LdapSearchFactory.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/Query.java PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/QueryFactory.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/SearchResultHandler.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/auth/ldap/UserFilterFactory.java 
PRE-CREATION 
  
service/src/java/org/apache/hive/service/auth/ldap/UserSearchFilterFactory.java 
PRE-CREATION 
  
service/src/test/org/apache/hive/service/auth/TestLdapAtnProviderWithMiniDS.java
 089a059 
  
service/src/test/org/apache/hive/service/auth/TestLdapAuthenticationProviderImpl.java
 f276906 
  service/src/test/org/apache/hive/service/auth/ldap/Credentials.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/LdapTestUtils.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/TestChainFilter.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/TestCustomQueryFilter.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/TestGroupFilter.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/TestLdapSearch.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/TestLdapUtils.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/TestQuery.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/TestQueryFactory.java 
PRE-CREATION 
  
service/src/test/org/apache/hive/service/auth/ldap/TestSearchResultHandler.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/TestUserFilter.java 
PRE-CREATION 
  service/src/test/org/apache/hive/service/auth/ldap/TestUserSearchFilter.java 
PRE-CREATION 

Diff: https://reviews.apache.org/r/51694/diff/


Testing
---

...hive/service> mvn clean test

...

Results :

Tests run: 123, Failures: 0, Errors: 0, Skipped: 0

[INFO] 
[INFO] BUILD SUCCESS
[INFO] 
[INFO] Total time: 04:18 min
[INFO] Finished at: 2016-09-06T08:46:04-07:00
[INFO] Final Memory: 66M/984M
[INFO] 


Thanks,

Illya Yalovyy