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, 201

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,

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 --

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 > > i

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 > > i

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, > > IndexOutO

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

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 f

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

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)

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 Szeho