Xiang Yang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19042 )
Change subject: IMPALA-11235: Support Pluggable Authentication for Impala ...................................................................... Patch Set 3: (9 comments) Thanks for your contribution, minghui! http://gerrit.cloudera.org:8080/#/c/19042/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19042/3//COMMIT_MSG@1 PS3, Line 1: Parent: 296e9441 (IMPALA-11599/IMPALA-11605: GCC 10: Fix gdb and change to flatbuffers 1.9) can we give this authentication implementation a more explict name instead of the 'plugin'? such as commandline authentication .etc. can you add docs like 'https://impala.apache.org/docs/build/html/topics/impala_ldap.html'? http://gerrit.cloudera.org:8080/#/c/19042/3/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/19042/3/be/src/rpc/authentication.cc@309 PS3, Line 309: LOG(ERROR) << "Plugin authentication failed for empty password。"; nit: don't use chinese punctuation. http://gerrit.cloudera.org:8080/#/c/19042/3/bin/plugin_for_test.sh File bin/plugin_for_test.sh: http://gerrit.cloudera.org:8080/#/c/19042/3/bin/plugin_for_test.sh@1 PS3, Line 1: #!/usr/bin/env bash lack of apache License header. http://gerrit.cloudera.org:8080/#/c/19042/3/bin/plugin_for_test.sh@18 PS3, Line 18: exit 1 use a different exit code to distinguish from line 4. http://gerrit.cloudera.org:8080/#/c/19042/3/fe/src/test/java/org/apache/impala/customcluster/PluginHS2Test.java File fe/src/test/java/org/apache/impala/customcluster/PluginHS2Test.java: http://gerrit.cloudera.org:8080/#/c/19042/3/fe/src/test/java/org/apache/impala/customcluster/PluginHS2Test.java@1 PS3, Line 1: // or more contributor license agreements. See the NOTICE file Incomplete lincense header. http://gerrit.cloudera.org:8080/#/c/19042/3/fe/src/test/java/org/apache/impala/customcluster/PluginHS2Test.java@53 PS3, Line 53: * plugin authentication is being used. nit: is used. http://gerrit.cloudera.org:8080/#/c/19042/3/fe/src/test/java/org/apache/impala/customcluster/PluginHS2Test.java@67 PS3, Line 67: static void verifySuccess(TStatus status) throws Exception { can use org.apache.hive.jdbc.Utils.verifySuccess() instead. http://gerrit.cloudera.org:8080/#/c/19042/3/fe/src/test/java/org/apache/impala/customcluster/PluginHS2Test.java@155 PS3, Line 155: transport.open(); do we need close THttpClient at the end of the test? http://gerrit.cloudera.org:8080/#/c/19042/3/fe/src/test/java/org/apache/impala/customcluster/PluginImpylaHttpTest.java File fe/src/test/java/org/apache/impala/customcluster/PluginImpylaHttpTest.java: http://gerrit.cloudera.org:8080/#/c/19042/3/fe/src/test/java/org/apache/impala/customcluster/PluginImpylaHttpTest.java@38 PS3, Line 38: * Impyla HTTP connectivity tests with LDAP authentication. What's the difference with LdapImpylaHttpTest? I see many Duplicate codes, can we refactor it to base class and inherited class mode? -- To view, visit http://gerrit.cloudera.org:8080/19042 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieaaef6bffe641ece30e7fc3756688ef30722dfe1 Gerrit-Change-Number: 19042 Gerrit-PatchSet: 3 Gerrit-Owner: Minghui Zhu <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jian Zhang <[email protected]> Gerrit-Reviewer: Minghui Zhu <[email protected]> Gerrit-Reviewer: Xiang Yang <[email protected]> Gerrit-Comment-Date: Fri, 30 Sep 2022 03:47:56 +0000 Gerrit-HasComments: Yes
