Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15570 )
Change subject: IMPALA-2563: Support LDAP search bind operations ...................................................................... Patch Set 4: Code-Review+2 (4 comments) http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/util/ldap-util.cc File be/src/util/ldap-util.cc: http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/util/ldap-util.cc@121 PS3, Line 121: user_filter_ = Split(user_filter, ","); > Its already the case that we can't handle usernames with commas (or any oth It's best not to mix that fix into this patch. It seems like a low priority TBH. http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/util/ldap-util.cc@133 PS3, Line 133: // DN patterns and replacing the optional '%s' with each group name, if present. > Yes. I was intending to follow Hive's example as closely as possible, and +1 to that plan. http://gerrit.cloudera.org:8080/#/c/15570/4/be/src/util/ldap-util.cc File be/src/util/ldap-util.cc: http://gerrit.cloudera.org:8080/#/c/15570/4/be/src/util/ldap-util.cc@235 PS4, Line 235: // Construct a filter that will search for LDAP entries that represent groups Thank you! These are very helpful. http://gerrit.cloudera.org:8080/#/c/15570/4/be/src/util/ldap-util.cc@266 PS4, Line 266: ldap_msgfree(result); How did you find what I assume is a memory leak here? I took a closer look at the code to try and spot similar problems but it is quite error prone. -- To view, visit http://gerrit.cloudera.org:8080/15570 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7502a96e9a3c16faa67c03ffac54df2bdebbca8c Gerrit-Change-Number: 15570 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 31 Mar 2020 19:11:06 +0000 Gerrit-HasComments: Yes
