Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/15219 )
Change subject: IMPALA-9384: Improve Impala shell usability by automatically enable live_progress in the interactive mode ...................................................................... Patch Set 1: (8 comments) This looks like a good change, thanks. I have a few nits and questions. http://gerrit.cloudera.org:8080/#/c/15219/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15219/1//COMMIT_MSG@8 PS1, Line 8: live_progress in the interactive mode I think the single line starting with 'IMPALA-9384' should be a short single line summary, so it should not overflow onto the next line. http://gerrit.cloudera.org:8080/#/c/15219/1//COMMIT_MSG@12 PS1, Line 12: mode when users do not set the option in both configure file and command Should this be "in either the configuration file or as a command line flag"? http://gerrit.cloudera.org:8080/#/c/15219/1//COMMIT_MSG@19 PS1, Line 19: Add a documentation jira to update docs for the change, or refer to it in the commit message if you already added it. http://gerrit.cloudera.org:8080/#/c/15219/1/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/15219/1/shell/impala_shell.py@1728 PS1, Line 1728: # config_file_live_progress is None, which means user didn't set live_progress Nit: maybe clearer to say "Set config_file_live_progress to None" ... http://gerrit.cloudera.org:8080/#/c/15219/1/shell/impala_shell.py@1742 PS1, Line 1742: config_file_live_progress = s_options.get('live_progress') As this is being set in a loop, is it possible that the value can be set to something the first time though, and then overwritten back to None the next time through? http://gerrit.cloudera.org:8080/#/c/15219/1/shell/impala_shell.py@1753 PS1, Line 1753: # live_progress in the config file, Impala shell will automatically enable it Nit: end with a period http://gerrit.cloudera.org:8080/#/c/15219/1/shell/impala_shell.py@1754 PS1, Line 1754: if not options.query and not options.query_file and config_file_live_progress is None: Can we simplify by using 'self.interactive' here? http://gerrit.cloudera.org:8080/#/c/15219/1/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/15219/1/tests/shell/test_shell_interactive.py@503 PS1, Line 503: # file and command line, Impala shell will automatically enable it Nit: end comment with a period -- To view, visit http://gerrit.cloudera.org:8080/15219 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3765b775f663fa227e59728acffe4d5ea9a5e2d3 Gerrit-Change-Number: 15219 Gerrit-PatchSet: 1 Gerrit-Owner: Alice Fan <fan...@gmail.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 24 Feb 2020 19:02:32 +0000 Gerrit-HasComments: Yes
