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 4: (6 comments) first pass, i'm not really familiar with our python infra, so it might take me a while to go through this properly; left a few preliminary comments http://gerrit.cloudera.org:8080/#/c/15524/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15524/4//COMMIT_MSG@17 PS4, Line 17: With regard to validating the patch, my assumption is that simply passing : the existing set of e2e shell tests is sufficient to confirm that the shell : is functioning properly. No new tests were added. given that the main pain here was dealing with the encodings, would it make sense to add additional tests that deal with this? http://gerrit.cloudera.org:8080/#/c/15524/4//COMMIT_MSG@119 PS4, Line 119: No effort has been made at this point to come up with a way to integrate do we have plans to do this? http://gerrit.cloudera.org:8080/#/c/15524/4/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/15524/4/shell/impala_shell.py@600 PS4, Line 600: try: : args = self.sanitise_input(args.decode('utf-8')) # python2 : except AttributeError: : args = self.sanitise_input(args) # python3 can we use the 'if sys.version_info.major == 2:' check here? http://gerrit.cloudera.org:8080/#/c/15524/4/shell/impala_shell.py@1254 PS4, Line 1254: # if filter(self.DML_REGEX.match, tokens): is_dml = True : if any(self.DML_REGEX.match(t) for t in tokens): was the first line suppose to be deleted? http://gerrit.cloudera.org:8080/#/c/15524/4/shell/impala_shell.py@1913 PS4, Line 1913: print(shell.CANCELLATION_MESSAGE, file=sys.stderr) : shell._reconnect_cancellation() : else: : print("Socket error %s: %s" % (e.errno, e)) what is this change for? http://gerrit.cloudera.org:8080/#/c/15524/4/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/15524/4/tests/shell/test_shell_interactive.py@803 PS4, Line 803: we expect users to also : # any install 3rd upstream libraries from PyPI. nit: revise -- 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: 4 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, 25 Mar 2020 21:51:00 +0000 Gerrit-HasComments: Yes
