Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 )
Change subject: IMPALA-10161: User LDAP Search bind support ...................................................................... Patch Set 7: (10 comments) Hi Csaba, thank you for the review! Updated the change. http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc File be/src/util/ldap-search-bind.cc: http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc@46 PS7, Line 46: std::string > Here and at other places: do you intentionally don't use "using namespace s I added it everywhere just in case. Revisited the includes/using directives and cleaned it up. The cpp files now use either the 'strings' namespace or the 'std::string'. 'strings' namespace uses the 'std::string'. http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc@53 PS7, Line 53: Status ldapBaseValidateStatus = ImpalaLdap::ValidateFlags(); : if (!ldapBaseValidateStatus.ok()) return ldapBaseValidateStatus; > We generally use the RETURN_IF_ERROR macro for this pattern. Done http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc@71 PS7, Line 71: Status ldapBaseInitStatus = ImpalaLdap::Init(user_filter, group_filter); : if (!ldapBaseInitStatus.ok()) return ldapBaseInitStatus; > RETURN_IF_ERROR Done http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc@100 PS7, Line 100: std::string > not needed, user_filter_ is already a string Done http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc@137 PS7, Line 137: group_filter_ > I think it would be a bit clearer to call find on group_filter instead of g Yes, agree, done. http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc@142 PS7, Line 142: if (user_dn.empty()) return false; > ldap_unbind_ext is not called if we return here Done http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-simple-bind.cc File be/src/util/ldap-simple-bind.cc: http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-simple-bind.cc@56 PS7, Line 56: Status ldapBaseValidateStatus = ImpalaLdap::ValidateFlags(); : if (!ldapBaseValidateStatus.ok()) return ldapBaseValidateStatus; > RETURN_IF_ERROR Done http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-simple-bind.cc@80 PS7, Line 80: Status ldapBaseInitStatus = ImpalaLdap::Init(user_filter, group_filter); : if (!ldapBaseInitStatus.ok()) return ldapBaseInitStatus; > RETURN_IF_ERROR Done http://gerrit.cloudera.org:8080/#/c/16717/7/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java File fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java: http://gerrit.cloudera.org:8080/#/c/16717/7/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java@46 PS7, Line 46: LdapSearchBindImpalaShellTest > I think that a lot of duplication could be potentially avoided with LdapSim Refactored the tests, moved the common methods to a base class. http://gerrit.cloudera.org:8080/#/c/16717/7/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java@291 PS7, Line 291: testLdapSearchBind > Can you make the name more descriptive? The whole file seems to be about te Done -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 7 Gerrit-Owner: Tamas Mate <tm...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Tue, 02 Feb 2021 21:55:11 +0000 Gerrit-HasComments: Yes
