Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13746 )
Change subject: IMPALA-8717: impala-shell support for HS2 HTTP endpoint ...................................................................... Patch Set 4: (6 comments) This is really cool. I didn't fully grok all the Thrift transport stuff yet, but thought I'd flush out comments at this point since I had some higher level feedback about the command line http://gerrit.cloudera.org:8080/#/c/13746/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13746/4//COMMIT_MSG@17 PS4, Line 17: impala-shell --protocol='hs2' --http (No auth) I kinda feel like we should be able to enable this with a single flag. That would be more convenient for users. There's now two options with two settings each, with one combination - beeswax/http that doesn't make sense. I think either we could make it a protocol - i.e. "hs2-http", or the --http flag could make the default protocol hs2. http://gerrit.cloudera.org:8080/#/c/13746/4/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/13746/4/be/src/service/impala-server.cc@2080 PS4, Line 2080: << " to server " << connection_context.server_name << " closed."; I think it might still be useful to log the number of associated sessions even if they're not closed. http://gerrit.cloudera.org:8080/#/c/13746/4/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/13746/4/shell/impala_client.py@367 PS4, Line 367: host_and_port = "%s:%s" % (self.impalad_host, self.impalad_port) nit: prefer format(), e.g. "{0}:{1}".format(self.impalad_host, self.impalad_port) http://gerrit.cloudera.org:8080/#/c/13746/4/shell/impala_client.py@377 PS4, Line 377: transport = THttpClient("https://%s"; % host_and_port, ssl_context=ssl_ctx) Same comment about format() http://gerrit.cloudera.org:8080/#/c/13746/4/shell/impala_client.py@379 PS4, Line 379: transport = THttpClient("http://%s"; % host_and_port) Same comment about format() http://gerrit.cloudera.org:8080/#/c/13746/4/shell/impala_client.py@383 PS4, Line 383: auth = base64.encodestring('%s:%s' % (self.user, self.ldap_password)).strip('\n') Same comment about format() -- To view, visit http://gerrit.cloudera.org:8080/13746 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8323950857dfe1c1dfd5377fde79f87bc2ce9534 Gerrit-Change-Number: 13746 Gerrit-PatchSet: 4 Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 28 Jun 2019 16:58:50 +0000 Gerrit-HasComments: Yes
