Sahil Takiar 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: (3 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 > Presumably. I just wanted to leave a safety valve for reverting to the prev yeah - i'm not sure this is adding much value, especially if this is just a dev facing config i vote for removing it and just always using the Thrift 11 code in impala-shell going forward (I think that is essentially what we are doing for actual end-users of the impala-shell, right?), it should make this patch simpler as well but maybe @Tim has some thoughts on this as well? 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 > This requirements file only pertains to the pip-installable python package so for the "pip-installable python package" we use 0.11.0, but for the "pip-installed shell" we use 0.9? how do we provide Python 3 support for the "pip-installed shell" if it still uses 0.9? 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: > When I had to update the string/unicode handling here, I took the opportuni i guess if each object is only ever used once, it is probably fine. on the other hand, unless there is a reason for making this change, i'm not sure what was wrong with the old approach. generally in favor of keeping the previous behavior unless there is a good reason to change it. -- 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: Wed, 08 Apr 2020 19:14:04 +0000 Gerrit-HasComments: Yes
