Vincent Tran has posted comments on this change. ( http://gerrit.cloudera.org:8080/7241 )
Change subject: IMPALA-2782: Allow impala-shell to connect directly to impalad when configured with load balancer and kerberos. ...................................................................... Patch Set 2: > Patch Set 2: > > I had to look at the JIRA to understand what this is trying to accomplish. > That suggests that the important change in impala_client.py (where you set up > the SASL) needs explanation. You've changed what Thrift/SASL/Kerberos is > expecting, and it's worth having a comment that explains the scenario. > > That said, I think Hrishikesh Gadre's comment on the JIRA is also worth > addressing. It feels weird that when load-balancers are configured, impalad > always has this issue (that it impersonates the load balancer?). Is that > condition relaxable? The change you have works here, but it would need to be > repeated for JDBC drivers and for impyla and anything else. > > As a nit, I think "self.lb" is a poor variable name; self.load_balancer is > better. Even there, I'd worry that the client is going to get confused: the > point of the load balancer is that it acts like an impalad, so I worry people > are going to specify both their impalad (?) and their load-balancer, even in > scenarios when they don't need to. These are quite user-visible flags. Good point on the lack of explanation. In the Simba JDBC driver, the equivalent variable is already exposed as KrbHostFQDN. In the ODBC driver, the field is exposed as "Host FQDN". Both of which are still somewhat confusing to a neophyte like myself. As for the variable name, perhaps we can use a similar name (i.e. self.KrbHostFQDN) for the sake of consistency. I can also lean the other way (i.e. self.load_balancer) since it is more appropriate for its purpose in my opinion. What are your thoughts? -- To view, visit http://gerrit.cloudera.org:8080/7241 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4726226a7a3817421b133f74dd4f4cf8c52135f9 Gerrit-Change-Number: 7241 Gerrit-PatchSet: 2 Gerrit-Owner: Vincent Tran <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Vincent Tran <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Comment-Date: Sun, 11 Feb 2018 02:54:00 +0000 Gerrit-HasComments: No
