Vincent Tran 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 4:

(5 comments)

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 
value passed in via -i / --impalad
> subjects shouldn't be wrapped, in theory.
Done


http://gerrit.cloudera.org:8080/#/c/10580/3//COMMIT_MSG@10
PS3, Line 10: that impala-shell starts the session displaying the expected
> Let's find a way to add a test for this scenario here.
Done

I think that to verify this scenario we must prove that the impala daemon 
target passed into -i/--impalad is always the one that the actual socket is 
created against.

I don't know if it really makes sense create a load balancer socket and listen 
on it in this scenario. The value that used in the -b option is just the 
hostname of the load balancer. The port doesn't really matter.

Checkout my test in the latest patchset. Let me know if it makes sense to you.


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_host = impalad[0].encode('ascii', 'ignore')
> It would be reasonable to introduce:
Yeah. This does make sense and adds more readability.


http://gerrit.cloudera.org:8080/#/c/10580/3/shell/impala_client.py@281
PS3, Line 281:     # the user. impala-shell checks to ensure this host matches 
the host in the kerberos
> "the its hostname" -> this doesn't parse to me. Can you clear it up?
Done


http://gerrit.cloudera.org:8080/#/c/10580/3/shell/impala_client.py@284
PS3, Line 284:     # connect directly to an impalad.
             :     if self.kerberos_host_fqdn is not None:
             :       sasl_host = 
self.kerberos_host_fqdn.split(':')[0].encode('ascii', 'ignore')
             :     else:
             :       sasl_host = self.impalad_host
> Please rename this to "sasl_host", since the only use is line 306.
Done



--
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: 4
Gerrit-Owner: Vincent Tran <[email protected]>
Gerrit-Reviewer: Philip Zeyliger <[email protected]>
Gerrit-Reviewer: Vincent Tran <[email protected]>
Gerrit-Reviewer: [email protected]
Gerrit-Comment-Date: Sat, 16 Jun 2018 23:28:29 +0000
Gerrit-HasComments: Yes

Reply via email to