Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/17047 )
Change subject: IMPALA-10161: User LDAP Search bind support ...................................................................... Patch Set 1: (3 comments) A few remaining nits, but otherwise it looks good to me. I'll give Csaba an opportunity to take another look if he wants before +2ing it http://gerrit.cloudera.org:8080/#/c/17047/1/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/17047/1/be/src/util/webserver.cc@128 PS1, Line 128: "Used as filter for both simple and " : "search nit: formatting (i.e. have the string start on its own line like it is here, but wrap it such that its as long as possible on that line), here and elsewhere unfortunately this is one of the things that clang-format gets wrong (since it won't automatically combine strings for you) http://gerrit.cloudera.org:8080/#/c/17047/1/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/17047/1/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java@104 PS1, Line 104: n typo http://gerrit.cloudera.org:8080/#/c/17047/1/fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java File fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java: http://gerrit.cloudera.org:8080/#/c/17047/1/fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java@42 PS1, Line 42: @Test I think you missed some instances of @Override here and below, though it might be more straightforward to just avoid overriding things by naming the functions in the base class something like test...Impl() or similar. Not a big deal, though. -- To view, visit http://gerrit.cloudera.org:8080/17047 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: 17047 Gerrit-PatchSet: 1 Gerrit-Owner: Tamas Mate <tm...@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, 09 Feb 2021 22:40:54 +0000 Gerrit-HasComments: Yes
