Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 )
Change subject: IMPALA-10161: User LDAP Search bind support ...................................................................... Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/16717/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16717/10//COMMIT_MSG@17 PS10, Line 17: ldap_search_bind ldap_search_bind_authentication? http://gerrit.cloudera.org:8080/#/c/16717/10/be/src/util/ldap-search-bind.cc File be/src/util/ldap-search-bind.cc: http://gerrit.cloudera.org:8080/#/c/16717/10/be/src/util/ldap-search-bind.cc@76 PS10, Line 76: << DEFAULT_USER_FILTER; There's a couple of formatting issues in the patch. It might be helpful if you ran clang-format-diff on it, per the instructions in https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536 http://gerrit.cloudera.org:8080/#/c/16717/10/be/src/util/webserver.h File be/src/util/webserver.h: http://gerrit.cloudera.org:8080/#/c/16717/10/be/src/util/webserver.h@29 PS10, Line 29: #include "util/ldap-search-bind.h" I might be missing something, but not sure why this should go here, as opposed to in the .cc file. http://gerrit.cloudera.org:8080/#/c/16717/10/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/16717/10/be/src/util/webserver.cc@451 PS10, Line 451: if (!FLAGS_ldap_search_bind_authentication) { : ldap_.reset(new LdapSimpleBind()); : } else { : ldap_.reset(new LdapSearchBind()); : } Might be cleaner to encapsulate this in something like a 'static void ImpalaLdap::CreateLdap(unique_ptr<ImpalaLdap>*)' or similar, and then use it in authentication.cc as well -- 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: 10 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: Sat, 06 Feb 2021 00:51:22 +0000 Gerrit-HasComments: Yes
