David Knupp 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: Had a long chat with Alice about IMPALA-9384. This was my high level understanding. * The primary goal is to change the default behavior when a user launches the impala-shell in interactive mode. Currently, live_progress is disabled by default, but we want it enabled by default. * There are currently two ways for an end user to enable the feature at runtime: either via a config file, or via the command line option --live_progress. * Changing the default to enabled has no bearing on using the shell with -q or -f, because live_progress does not apply to those use cases. In fact, we throw a FatalShellException (https://github.com/apache/impala/blob/master/shell/impala_shell.py#L1845) if live_progress is enabled in non-interactive mode (which actually seems a little overly aggressive to me; I think we could probably get away with a warning, but whatever). * In the event of a collision (i.e., config file tries to disable it, but command line option tries to enable it), the command line setting takes precedence. With these in mind, these are my recommendations: * If the default behavior is to have live_progress enabled by default in interactive mode, then we should provide an option on the command line for --disable_live_progress. * Since the feature can be disabled in the config file, but then overridden via the command line on a per-use basis, it probably makes sense to retain --live_progress as a command line option, even if it initially seems a little redundant. We could perhaps edit the help text to explain that this can be used to override settings from the config file. * If this were me, I would change the default setting at https://github.com/apache/impala/blob/master/shell/impala_shell_config_defaults.py#L42 to True. As it is, it's being left False and then implicitly changed to True later in the code. This happens because we currently throw an exception if live_progress=True is used with non-interactive. But what this means is that we have a setting in impala_shell_config_defaults.py that can ONLY ever be False, so really, why even expose it at all? Since live_progress doesn't even apply to non-interactive use of the shell, if you're going to be changing configs implicitly somewhere, I'd far prefer to implicitly disable if the non-interactive mode is detected. The whole business of throwing an exception seems like overkill. Finally, a small additional observation. My guess is that people most commonly use the shell interactively. I would guess that for automatd/scripted queries, Impyla/JDBC/ODBC are far more common than relying on -q or -f with the shell. I'm wondering what others' opinions are about this. -- 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 <[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: Fri, 28 Feb 2020 19:51:50 +0000 Gerrit-HasComments: No
