Alice Fan 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) Thanks for the feedback Andrew. 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 singl sure. will change. 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" sure. will update. 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 t sure. will file a documentation jira for 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" ... sure. will change 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 I think I didn't get what you mean here. can you please elaborate? thanks 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 thanks. will do 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? I think we are doing the if statement at main() and the self.interactive is defined as private variable of ImpalaShell class. So here we may have to do the same like https://github.com/apache/impala/blob/master/shell/impala_shell.py#L1842 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 Thanks. will change. -- 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: 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: Fri, 28 Feb 2020 18:40:25 +0000 Gerrit-HasComments: Yes
