[Impala-ASF-CR] IMPALA-9384: Improve Impala shell usability by automatically enable live progress in the interactive mode
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: > Patch Set 1: > David and Andrew, please let me know if you agree with option 2? Thanks! Thanks Alice -- Andrew and I both in favor of option 2. -- 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 Gerrit-Reviewer: Alice Fan Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 02 Mar 2020 17:42:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9384: Improve Impala shell usability by automatically enable live progress in the interactive mode
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:
(1 comment)
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@1742
PS1, Line 1742: config_file_live_progress = s_options.get('live_progress')
> Line 1731 set config_file_live_progress = None
Thanks for catching this. I left a comment for option 1 and 2 according to
David's feedback. Let's see which path we decide to go.
--
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
Gerrit-Reviewer: Alice Fan
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: David Knupp
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Fri, 28 Feb 2020 23:33:22 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9384: Improve Impala shell usability by automatically enable live progress in the interactive mode
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: Thanks David for your input. I would like to summarize the two possible options here: Option 1: current patch 1 + modification for --disable_live_progress Won’t set live_progress = true in impala_shell_config_default.py In impala_shell.py, we set live_progress to True when users neither set it in configuration file nor in command line AND it is in the interactive mode. Option 2: set live_progress to TRUE in impala_shell_config_default.py set live_progress to true in impala_shell_config_default.py Impala_shell implicitly disables live_progress when the non-interactive mode is detected even user set it to true in config file or/and command line Remove the check and throw FatalShellException for the case of detecting live_progress is true in non-interactive mode because of (2) Either choosing option 1 or 2, we can add “--disable_live_progress” This allows users to explicitly disable live_progress by command line when they are in interactive mode. To me, I think option 2 looks more straightforward. That is - in the non-interactive mode, live_progress will always be false no matter if a user tries to set it to true in config file and/or command line. Impala_shell will implicitly set it to false when detecting it is in non-interactive mode. As for interactive mode, by default live_progress is set to True, a user has options to set it to false in either command line or config file. David and Andrew, please let me know if you agree with option 2? Thanks! -- 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 Gerrit-Reviewer: Alice Fan Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 28 Feb 2020 23:17:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9384: Improve Impala shell usability by automatically enable live progress in the interactive mode
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:
(2 comments)
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@1742
PS1, Line 1742: config_file_live_progress = s_options.get('live_progress')
> I think I didn't get what you mean here. can you please elaborate? thanks
Line 1731 set config_file_live_progress = None
Line 1737 puts us in a loop through config files
- Say first config file has a value for 'live_progress', now on Line 1742
config_file_live_progress=True
- Say second config file does not have a value for 'live_progress', now on
Line 1742 config_file_live_progress=None again
So config_file_live_progress gets set then overwritten, that seems weird.
(Probably this doesn't matter if you rewrite as dknupp suggests)
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:
> I think we are doing the if statement at main() and the self.interactive is
OK
--
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
Gerrit-Reviewer: Alice Fan
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: David Knupp
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Fri, 28 Feb 2020 22:20:08 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9384: Improve Impala shell usability by automatically enable live progress in the interactive mode
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 Gerrit-Reviewer: Alice Fan Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 28 Feb 2020 19:51:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9384: Improve Impala shell usability by automatically enable live progress in the interactive mode
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: Hi David, Thanks for your feedback. As we discussed offline, please let me know when you have an update about how to proceed. Thanks -- 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 Gerrit-Reviewer: Alice Fan Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 28 Feb 2020 19:05:47 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9384: Improve Impala shell usability by automatically enable live progress in the interactive mode
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
Gerrit-Reviewer: Alice Fan
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: David Knupp
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Fri, 28 Feb 2020 18:40:25 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9384: Improve Impala shell usability by automatically enable live progress in the interactive mode
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:
> Patch Set 1:
>
> (8 comments)
>
> This looks like a good change, thanks.
> I have a few nits and questions.
I think there may be an issue with the fundamental approach here. In
shell/options_parser.py, we have this code:
https://github.com/apache/impala/blob/master/shell/option_parser.py#L235
parser.add_option("--live_progress", dest="live_progress",
action="store_true",
help="Print a query progress every 1s while the query is
running.")
This implies that the default setting for live_progress is False, and we need a
special flag to enable it. I would start by changing this flag to
--disable_live_progress (don't get me started on underscores vs hyphens for
command line options) with action="store_false", and see where that gets you.
--
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
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: David Knupp
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Tue, 25 Feb 2020 19:51:36 +
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9384: Improve Impala shell usability by automatically enable live progress in the interactive mode
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
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: David Knupp
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Mon, 24 Feb 2020 19:02:32 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9384: Improve Impala shell usability by automatically enable live progress in the interactive mode
Impala Public Jenkins 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: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5219/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 13 Feb 2020 23:50:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9384: Improve Impala shell usability by automatically enable live progress in the interactive mode
Alice Fan has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15219 Change subject: IMPALA-9384: Improve Impala shell usability by automatically enable live_progress in the interactive mode .. IMPALA-9384: Improve Impala shell usability by automatically enable live_progress in the interactive mode Currently, shell option live_progress is set to False by default. The patch makes Impala shell automatically enable it in the interactive mode when users do not set the option in both configure file and command line. This will help to improve usability by automatically showing the query processing status to users. Testing: - Updated and added tests in test_shell_interactive.py - Successfully ran all other shell interactive related tests Change-Id: I3765b775f663fa227e59728acffe4d5ea9a5e2d3 --- M shell/impala_shell.py M tests/shell/test_shell_interactive.py 2 files changed, 15 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/15219/1 -- 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: newchange Gerrit-Change-Id: I3765b775f663fa227e59728acffe4d5ea9a5e2d3 Gerrit-Change-Number: 15219 Gerrit-PatchSet: 1 Gerrit-Owner: Alice Fan Gerrit-Reviewer: Tim Armstrong
