Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/10580 )
Change subject: IMPALA-7130: impala-shell -b / --kerberos_host_fqdn flag overrides value passed in via -i / --impalad ...................................................................... Patch Set 3: (5 comments) I think this is close. I could be swayed on the test. http://gerrit.cloudera.org:8080/#/c/10580/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10580/3//COMMIT_MSG@7 PS3, Line 7: IMPALA-7130: impala-shell -b / --kerberos_host_fqdn flag overrides subjects shouldn't be wrapped, in theory. http://gerrit.cloudera.org:8080/#/c/10580/3//COMMIT_MSG@10 PS3, Line 10: After additional testing around IMPALA-2782, it was discovered Let's find a way to add a test for this scenario here. We can't fully test it because we don't have a kerberos environment, but we can at least figure out if the right socket is opened. Use Python or netcat to start listening on two ports, A (the load balancer) and B (the actual impalad). With one set of impala-shell options, it should try to connect to A and with another set it should try to connect to B. We can see if A and B have anything to read. Let me know if you think that's unreasonable effort-wise. http://gerrit.cloudera.org:8080/#/c/10580/3/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/10580/3/shell/impala_client.py@66 PS3, Line 66: self.impalad = impalad It would be reasonable to introduce: self.impalad_host = impalad[0].encode(...) self.impalad_port = int(impalad[1]) here I don't insist. http://gerrit.cloudera.org:8080/#/c/10580/3/shell/impala_client.py@281 PS3, Line 281: # principal. So in the presence of a load balancer, the its hostname is expected by "the its hostname" -> this doesn't parse to me. Can you clear it up? http://gerrit.cloudera.org:8080/#/c/10580/3/shell/impala_client.py@284 PS3, Line 284: if self.kerberos_host_fqdn is not None: : host, port = (self.kerberos_host_fqdn.split(':')[0].encode('ascii', 'ignore'), : int(self.impalad[1])) : else: : host, port = self.impalad[0].encode('ascii', 'ignore'), int(self.impalad[1]) Please rename this to "sasl_host", since the only use is line 306. Please move the "port = int(self.impalad[1])" part of this to line 292 and call it "sock_port". I had to think a little bit too hard about why you didn't need a port number for the load balancer, and, in part, I think that helped obscure the bug. -- To view, visit http://gerrit.cloudera.org:8080/10580 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibee05bd0dbe8c6ae108b890f0ae0f6900149773a Gerrit-Change-Number: 10580 Gerrit-PatchSet: 3 Gerrit-Owner: Vincent Tran <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Comment-Date: Fri, 15 Jun 2018 15:55:10 +0000 Gerrit-HasComments: Yes
