David Knupp 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: (5 comments) 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 this value has been changed on-the-fly, e.g. print_to_stderr("Warning: live_progress only applies to interactive shell sessions, and is being skipped for now") ...or something like that. 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 to false here as well. For the moment, let's at least change "Live reporting" to "live_summary", since I suspect the former term was intentionally non-specific so as to cover both live_summary and live_progress. 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 only applies to interactive shell sessions. 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 think? " If live_progress is set to False in a config file, this flag will override it." 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. The way that we set defaults in the impala-shell is, I think, not customary. Both optparse (which we use, but that is in fact deprecated) and argparse allow one to specify a default value by passing a default argument. e.g., parser.add_option("--my-flag", action="store_false", dest="my_setting", default=True, help="blah blah blah") Since we don't do that, I suspect this check isn't required. However, one check you want to do is to see what happens if someone unintentionally passes both --live_progress and --disable_live_progress at the same time. I think the preferred behavior is to raise a parser error, e.g. if options.live_progress and options.disable_live_progress: parser.error("options --live_progress and --disable_live_progress are mutually exclusive") ...or something like that. -- 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 <[email protected]> Gerrit-Reviewer: Alice Fan <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 03 Mar 2020 18:35:03 +0000 Gerrit-HasComments: Yes
