impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Knupp (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3343, IMPALA-9489: Make impala-shell compatible with python 3.
Date Tue, 07 Apr 2020 20:31:45 GMT
David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/15524 )

Change subject: IMPALA-3343, IMPALA-9489: Make impala-shell compatible with python 3.
......................................................................


Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/15524/6/bin/impala-shell.sh
File bin/impala-shell.sh:

http://gerrit.cloudera.org:8080/#/c/15524/6/bin/impala-shell.sh@27
PS6, Line 27: USE_THRIFT11_GEN_PY
> who sets this? the user?
Presumably. I just wanted to leave a safety valve for reverting to the previous version of
thrift in the dev environment, but to be honest, I'm wondering if it's even necessary.


http://gerrit.cloudera.org:8080/#/c/15524/6/shell/packaging/requirements.txt
File shell/packaging/requirements.txt:

http://gerrit.cloudera.org:8080/#/c/15524/6/shell/packaging/requirements.txt@8
PS6, Line 8: thrift==0.11.0
> a bit confused about the version of Thrift being used. after this patch, wi
This requirements file only pertains to the pip-installable python package that a user might
independently install outside of a cluster -- e.g., on their laptop or whatever. In this case,
it's always thrift 0.11.0. Honestly, for the pip-installed shell, we could easily use a later
version than 0.11.0, but I wanted it to match the version that comes with CDH/CDP.


http://gerrit.cloudera.org:8080/#/c/15524/6/shell/shell_output.py
File shell/shell_output.py:

http://gerrit.cloudera.org:8080/#/c/15524/6/shell/shell_output.py@101
PS6, Line 101:         with open(self.filename, 'ab') as out_file:
> this seems to change the logic so that it opens and closed the file during 
When I had to update the string/unicode handling here, I took the opportunity to refactor
the class. I'm not wholly sure the result is behaving any differently. Every time the impala-shell
calls self.output_stream.write(), it seems to always be preceded by an instantiation of a
new OutputStream instance, so I think the object is used once and destroyed. It doesn't look
we ever cache the object long term. This may reflect that fact that, presumably, one can change
the name of the output_file in an already running shell session.

Am I wrong about this though?


http://gerrit.cloudera.org:8080/#/c/15524/6/tests/shell/test_shell_commandline.py
File tests/shell/test_shell_commandline.py:

http://gerrit.cloudera.org:8080/#/c/15524/6/tests/shell/test_shell_commandline.py@472
PS6, Line 472:     # args = ['-q', "select '{0}'".format(RUSSIAN_CHARS)]
> delete?
Ack



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb004d352fe230a890a6b6356496ba76c2fab615
Gerrit-Change-Number: 15524
Gerrit-PatchSet: 6
Gerrit-Owner: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <arawat@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenkins@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <stakiar@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Apr 2020 20:31:45 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message