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: (2 comments) 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 > so for the "pip-installable python package" we use 0.11.0, but for the "pip I'm confused -- there's only one thing that's pip installed, and that uses thrift==0.11.0 (from the line in this file). What is this referring to? "for the pip-installed shell we use 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: > i guess if each object is only ever used once, it is probably fine. on the Generally speaking, it's just rare to see a python class with a __del__() method, and if it's sole purpose is simply to call close() on a file handle, it sort of screams python-written-by-non-python-programmer. E.g., have a look at the most upvoted answer on this SO entry: https://stackoverflow.com/questions/2433130/how-do-constructors-and-destructors-work/ OTOH, I've been guilty of being overly pedantic about this kind of python-y thing in the past. My preference would be to change it, but I'll defer to a strongly held counter-opinion. -- 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: Thu, 09 Apr 2020 00:09:33 +0000 Gerrit-HasComments: Yes
