Alice Fan has posted comments on this change. ( http://gerrit.cloudera.org:8080/15219 )
Change subject: IMPALA-9384: Improve Impala shell usability by enabling live_progress in interactive mode ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/15219/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15219/3//COMMIT_MSG@9 PS3, Line 9: In order to improve usability, this patch would like to make Impala shell show query > The commit message describes the change. So "this patch makes Impala Shell Thanks. will update. http://gerrit.cloudera.org:8080/#/c/15219/3//COMMIT_MSG@11 PS3, Line 11: live_progress by default when a user launches impala shell in the interactive mode. > Nit: in general, unless you have some special text, you should word wrap at Sure. will do. Thanks http://gerrit.cloudera.org:8080/#/c/15219/3//COMMIT_MSG@14 PS3, Line 14: live_progress by either using the proposed command line flag or setting the option as > Don't say 'proposed' the commit message describes what changed. Thanks. will remove 'proposed' http://gerrit.cloudera.org:8080/#/c/15219/3/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/15219/3/shell/impala_shell.py@1845 PS3, Line 1845: options.live_progress = False > It might be nice to issue a message to the console here indicating that thi Thanks, will add warning message here. http://gerrit.cloudera.org:8080/#/c/15219/3/shell/impala_shell.py@1847 PS3, Line 1847: print_to_stderr("Error: Live reporting is available for interactive mode only.") > Of course, now this makes me wonder whether we should just set live_summary sure. will do http://gerrit.cloudera.org:8080/#/c/15219/3/shell/impala_shell_config_defaults.py File shell/impala_shell_config_defaults.py: http://gerrit.cloudera.org:8080/#/c/15219/3/shell/impala_shell_config_defaults.py@42 PS3, Line 42: 'live_progress': True, > Nit: you could add a comment above or beside here, indicating that this onl sure. will do http://gerrit.cloudera.org:8080/#/c/15219/3/shell/option_parser.py File shell/option_parser.py: http://gerrit.cloudera.org:8080/#/c/15219/3/shell/option_parser.py@238 PS3, Line 238: " Users can disable it by setting it as False in config file.") > I think that last sentence might be more clear like this -- what do you thi Thanks. will update this http://gerrit.cloudera.org:8080/#/c/15219/3/shell/option_parser.py@307 PS3, Line 307: elif option == parser.get_option('--disable_live_progress'): > Technically, I don't think this is necessary. I think the reason I added this line is because we want to make the help text's default value of --disable_live_progress is False For example: run impala-shell.sh --help --disable_live_progress A command line flag allows users to disable live_progress in the interactive mode. [default: False] // This need to be False and which is opposite to --live_progress = True Because --disable_live_progress and --live_progress shared the same dest="live_progress". The help text will print the default value of the destination flag, and we need to make sure it will print the opposite value. As for the two mutually exclusive flags are used case. I think the last one always take precedence. Ex: If a user run impala-shell.sh --verbose --quiet Impala shell will set verbose = false. This looks like be a consistent behavior and may not need a error to handle it? -- 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: 3 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: Tue, 03 Mar 2020 22:19:30 +0000 Gerrit-HasComments: Yes
