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 <dkn...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 07 Apr 2020 20:31:45 +0000 Gerrit-HasComments: Yes
