Bikramjeet Vig has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17667 )

Change subject: IMPALA-10784: Add support for retaining cookies in impala-shell
......................................................................


Patch Set 3:

(4 comments)

Looks good, just a few nits

http://gerrit.cloudera.org:8080/#/c/17667/3/shell/ImpalaHttpClient.py
File shell/ImpalaHttpClient.py:

http://gerrit.cloudera.org:8080/#/c/17667/3/shell/ImpalaHttpClient.py@66
PS3, Line 66: http_cookie_names is comma-separated list of cookie names which 
are used to specify
            :     the cookie-based authentication or session management
nit: http_cookie_names is used to specify a comma-separated list of possible 
cookie names used for cookie-based authentication  or session management. If a 
cookie with one of these names is returned in an http response by the server or 
an intermediate proxy then it will be included in each subsequent request for 
the same connection.


http://gerrit.cloudera.org:8080/#/c/17667/3/shell/ImpalaHttpClient.py@170
PS3, Line 170: updateHttpCookie
nit: updateHttpCookies


http://gerrit.cloudera.org:8080/#/c/17667/3/shell/ImpalaHttpClient.py@184
PS3, Line 184: self.__http_cookie_dict[cn] = Cookie(cookie=None, 
expiry_time=None)
nit: can we just remove the entry from __http_cookie_dict?


http://gerrit.cloudera.org:8080/#/c/17667/3/shell/ImpalaHttpClient.py@194
PS3, Line 194: addHttpCookie
nit: addHttpCookiesToRequestHeaders



--
To view, visit http://gerrit.cloudera.org:8080/17667
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I193422d5ec891886a522d82ecb0e9d974132ff2a
Gerrit-Change-Number: 17667
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <[email protected]>
Gerrit-Reviewer: Bikramjeet Vig <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Wenzhe Zhou <[email protected]>
Gerrit-Comment-Date: Fri, 09 Jul 2021 19:39:14 +0000
Gerrit-HasComments: Yes

Reply via email to