Steve Carlin 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 3: (14 comments) http://gerrit.cloudera.org:8080/#/c/17660/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17660/3//COMMIT_MSG@9 PS3, Line 9: Impala-shell already uses HS2 protocol to connect to Impalad. > nit: extra whitespace Done http://gerrit.cloudera.org:8080/#/c/17660/3//COMMIT_MSG@10 PS3, Line 10: any server > Can you add "for example Hive"? Done http://gerrit.cloudera.org:8080/#/c/17660/3//COMMIT_MSG@11 PS3, Line 11: --strict > I would prefer a clearer name, like strict_hs2 or no_hs2_extensions. Strict Done http://gerrit.cloudera.org:8080/#/c/17660/3//COMMIT_MSG@12 PS3, Line 12: > Can you add more info about what this client is capable of in strict mode? Done http://gerrit.cloudera.org:8080/#/c/17660/3//COMMIT_MSG@13 PS3, Line 13: When the "--strict" option is turned on > Wouldn't it be possible to detect this somehow at runtime? For example by c I don't think this is gonna be possible. The default mode in Impala creates a vanilla TBufferedTransport object and passes it into thrift. I tried this mode on HiveServer2 and it did not work. So the only way to really test the default is to experiment on connection, but I don't think that's a good idea since it would fail to connect the first time, and that would affect the number of attempts tried for a server. http://gerrit.cloudera.org:8080/#/c/17660/3/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/17660/3/shell/impala_client.py@1091 PS3, Line 1091: resp = self._do_hs2_rpc(CloseOperation, retry_on_error=True) > Not sure if we should retry here. The only case that is worth retrying is i Done http://gerrit.cloudera.org:8080/#/c/17660/3/shell/impala_client.py@1105 PS3, Line 1105: return "" > Shouldn't we assert when calling unsupported functions? Will this be actual This is called from impala_shell as a virtual method, so I can't assert here. http://gerrit.cloudera.org:8080/#/c/17660/3/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/17660/3/shell/impala_shell.py@191 PS3, Line 191: test_mode > Do we use test mode for anything else than setting ldap_password? if not, t Ok, so I thought about this... The problem I ran into is that the adding of the ldap_password was a bit awkward. If we pass in an ldap_password, impala-shell thinks we're in ldap mode. So if we wanted to be in kerberos mode, it doesn't really work to pass in the ldap password. The logic would have to be that if we are in strict mode and not in kerberos mode, send in the password for all calls to impala-shell. The logic seemed a bit too awkward since the kerberos decision is made within the top level file (e.g. test_shell_commandline) whereas the ldap_password decision would either have to be in every test method (ick) or within util.py when the command is generated, and that logic seemed a bit icky to me. By passing this "--test_mode", I think things came out the cleanest. But I'm willing to change if you think there's another way. http://gerrit.cloudera.org:8080/#/c/17660/3/shell/impala_shell.py@574 PS3, Line 574: 'steve', 'password > Will this work on other machines? Whoops! http://gerrit.cloudera.org:8080/#/c/17660/3/shell/impala_shell_config_defaults.py File shell/impala_shell_config_defaults.py: http://gerrit.cloudera.org:8080/#/c/17660/3/shell/impala_shell_config_defaults.py@58 PS3, Line 58: 'strict_protocol': False, > strict_hs2_protocol Done http://gerrit.cloudera.org:8080/#/c/17660/3/shell/option_parser.py File shell/option_parser.py: http://gerrit.cloudera.org:8080/#/c/17660/3/shell/option_parser.py@280 PS3, Line 280: parser.add_option("--strict_protocol", dest="strict_protocol", action="store_true", > strict_hs2_protocol. Make it clear this only applies to HS2. Done http://gerrit.cloudera.org:8080/#/c/17660/3/tests/shell/test_shell_commandline.py File tests/shell/test_shell_commandline.py: http://gerrit.cloudera.org:8080/#/c/17660/3/tests/shell/test_shell_commandline.py@142 PS3, Line 142: if vector.get_value('strict_protocol') and vector.get_value('protocol') == 'beeswax': : pytest.skip("Beeswax protocol not supported in strict mode.") > you can add this logic as a skipIf annotation in https://github.com/apache/ I figured out I can get rid of this via the "add_constraint" function http://gerrit.cloudera.org:8080/#/c/17660/3/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/17660/3/tests/shell/test_shell_interactive.py@193 PS3, Line 193: if vector.get_value('strict_protocol') and vector.get_value('protocol') == 'beeswax': > May be cleaner to split the tests into 2 vectors. I think I figured out a way to get rid of this. By calling add_constraint when adding dimensions, I can test for this specific condition and always skip it. http://gerrit.cloudera.org:8080/#/c/17660/3/tests/shell/util.py File tests/shell/util.py: http://gerrit.cloudera.org:8080/#/c/17660/3/tests/shell/util.py@318 PS3, Line 318: f = open('/home/steve/tmp/sjc24.txt', 'w') > change hard-coded path. Done -- 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: 3 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: Tue, 20 Jul 2021 21:00:43 +0000 Gerrit-HasComments: Yes
