Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/17660 )
Change subject: IMPALA-10778: Allow impala-shell to connect directly to HS2 ...................................................................... Patch Set 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/17660/7/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/17660/7/shell/impala_shell.py@192 PS7, Line 192: self.ldap_password = 'password' can you add a comment about the reason for using a dummy password? http://gerrit.cloudera.org:8080/#/c/17660/7/shell/impala_shell.py@574 PS7, Line 574: return StrictHS2Client(self.impalad, self.fetch_size, self.kerberos_host_fqdn, : self.use_kerberos, self.kerberos_service_name, self.use_ssl, : self.ca_cert, self.user, self.ldap_password, True, : self.client_connect_timeout_ms, self.verbose, : use_http_base_transport=False, http_path=self.http_path, : http_cookie_names=None) optional: I think that using a member like is_strict_mode could be a better option than having a subclass - my concern is that someone may change ImpalaHS2Client but forget to update StrictHS2Client. E.g. adding a new parameter to the constructor, but not adding it to line 574/581. What do you think? http://gerrit.cloudera.org:8080/#/c/17660/7/shell/option_parser.py File shell/option_parser.py: http://gerrit.cloudera.org:8080/#/c/17660/7/shell/option_parser.py@283 PS7, Line 283: "Only useful if connecting straight to hs2 instead of Impala.") Can you mention that 10001 is the default port in this case? (btw, what is the reason for 10001?) http://gerrit.cloudera.org:8080/#/c/17660/7/shell/option_parser.py@284 PS7, Line 284: test_mode I would prefer a more exact name like --use_ldap_test_password. We use impala shell in many unit tests without using this flag. -- To view, visit http://gerrit.cloudera.org:8080/17660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I674a45640a4a7b3c9a577830dbc7b16a89865a9e Gerrit-Change-Number: 17660 Gerrit-PatchSet: 7 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Mon, 09 Aug 2021 19:13:44 +0000 Gerrit-HasComments: Yes
