Details
-
Improvement
-
Status: Closed
-
Critical
-
Resolution: Fixed
-
None
-
None
-
Reviewed
-
HideIntroduces a new option to the HBase shell: -r, --return-values. When the shell is in "interactive" mode (default), the return value of shell commands are not returned to the user as they dirty the console output. For those who desire this functionality, the "--return-values" option restores the old functionality of the commands passing their return value to the user.ShowIntroduces a new option to the HBase shell: -r, --return-values. When the shell is in "interactive" mode (default), the return value of shell commands are not returned to the user as they dirty the console output. For those who desire this functionality, the "--return-values" option restores the old functionality of the commands passing their return value to the user.
Description
Another good find by our Romil.
hbase(main):001:0> list TABLE a 1 row(s) Took 0.8385 seconds hbase(main):002:0> tables=list TABLE a 1 row(s) Took 0.0267 seconds hbase(main):003:0> puts tables hbase(main):004:0> p tables nil
The list command should be returning ['a'] but is not.
The command class itself appears to be doing the right thing – maybe the retval is getting lost somewhere else?
FYI stack.
Attachments
Attachments
- HBASE-19770.000.branch-2.addendum.patch
- 0.8 kB
- Josh Elser
- HBASE-19770.001.branch-2.patch
- 3 kB
- Josh Elser
- HBASE-19770.002.branch-2.patch
- 4 kB
- Josh Elser
- HBASE-19770.003.branch-2.patch
- 5 kB
- Josh Elser
- HBASE-19770.004.branch-2.patch
- 5 kB
- Josh Elser
- HBASE-19770.ADDENDUM.branch-2.patch
- 0.8 kB
- Josh Elser
Issue Links
- is broken by
-
HBASE-15965 Shell test changes. Use @shell.command instead directly calling functions in admin.rb and other libraries.
- Resolved
- is related to
-
HBASE-20304 Flip default "return-values" in shell from false to true
- Closed
- is superceded by
-
HBASE-20276 [shell] Revert shell REPL change and document
- Closed
Activity
I'm not sure I'd agree with the assertion that "all interactive shells do not want return values".
Thinking some more on this: this change forces users into having their script fail on the first error. There's no option for me to say "Please give me command return values, but don't kill my shell session on command error."
.001 Here's what I was thinking instead:
Creates a new option -r | --return-values which controls whether or not the caller will get return values from shell commands.
This gives interactive shells the option to get return_values (my goal), but am wondering if there is ever a reason that non-interactive shells would not want return_values...
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 9s | Docker mode activated. |
Prechecks | |||
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
-1 | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
branch-2 Compile Tests | |||
0 | mvndep | 0m 10s | Maven dependency ordering for branch |
+1 | mvninstall | 3m 32s | branch-2 passed |
+1 | javadoc | 2m 34s | branch-2 passed |
Patch Compile Tests | |||
0 | mvndep | 0m 10s | Maven dependency ordering for patch |
+1 | mvninstall | 3m 32s | the patch passed |
-1 | rubocop | 0m 7s | The patch generated 3 new + 48 unchanged - 2 fixed = 51 total (was 50) |
+1 | ruby-lint | 0m 2s | There were no new ruby-lint issues. |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | javadoc | 2m 32s | the patch passed |
Other Tests | |||
-1 | unit | 138m 53s | root in the patch failed. |
+1 | asflicense | 0m 38s | The patch does not generate ASF License warnings. |
152m 38s |
Reason | Tests |
---|---|
Failed junit tests | hadoop.hbase.client.TestShell |
hadoop.hbase.client.TestReplicationShell |
Subsystem | Report/Notes |
---|---|
Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hbase:9f2f2db |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12905736/HBASE-19770.001.branch-2.patch |
Optional Tests | asflicense rubocop ruby_lint javac javadoc unit |
uname | Linux 89f4c3c43d8b 3.13.0-129-generic #178-Ubuntu SMP Fri Aug 11 12:48:20 UTC 2017 x86_64 GNU/Linux |
Build tool | maven |
Personality | /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/component/dev-support/hbase-personality.sh |
git revision | branch-2 / 0dacdde0f9 |
maven | version: Apache Maven 3.5.2 (138edd61fd100ec658bfa2d307c43b76940a5d7d; 2017-10-18T07:58:13Z) |
Default Java | 1.8.0_151 |
rubocop | v0.52.1 |
rubocop | https://builds.apache.org/job/PreCommit-HBASE-Build/11027/artifact/patchprocess/diff-patch-rubocop.txt |
ruby-lint | v2.3.1 |
unit | https://builds.apache.org/job/PreCommit-HBASE-Build/11027/artifact/patchprocess/patch-unit-root.txt |
Test Results | https://builds.apache.org/job/PreCommit-HBASE-Build/11027/testReport/ |
modules | C: hbase-shell . U: . |
Console output | https://builds.apache.org/job/PreCommit-HBASE-Build/11027/console |
Powered by | Apache Yetus 0.6.0 http://yetus.apache.org |
This message was automatically generated.
.002 Fix ruby "style" issues. Make sure return_values=true for the ruby tests too (root-cause of test failures from .001)
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 11s | Docker mode activated. |
Prechecks | |||
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 2 new or modified test files. |
branch-2 Compile Tests | |||
0 | mvndep | 0m 13s | Maven dependency ordering for branch |
+1 | mvninstall | 3m 39s | branch-2 passed |
+1 | javadoc | 2m 40s | branch-2 passed |
Patch Compile Tests | |||
0 | mvndep | 0m 11s | Maven dependency ordering for patch |
+1 | mvninstall | 3m 44s | the patch passed |
-1 | rubocop | 0m 12s | The patch generated 5 new + 84 unchanged - 2 fixed = 89 total (was 86) |
-1 | ruby-lint | 0m 4s | The patch generated 1 new + 61 unchanged - 0 fixed = 62 total (was 61) |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | javadoc | 3m 30s | the patch passed |
Other Tests | |||
+1 | unit | 137m 47s | root in the patch passed. |
+1 | asflicense | 0m 37s | The patch does not generate ASF License warnings. |
153m 7s |
Subsystem | Report/Notes |
---|---|
Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hbase:9f2f2db |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12905941/HBASE-19770.002.branch-2.patch |
Optional Tests | asflicense rubocop ruby_lint javac javadoc unit |
uname | Linux 3b4539555c2d 3.13.0-129-generic #178-Ubuntu SMP Fri Aug 11 12:48:20 UTC 2017 x86_64 GNU/Linux |
Build tool | maven |
Personality | /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/component/dev-support/hbase-personality.sh |
git revision | branch-2 / 026f535a77 |
maven | version: Apache Maven 3.5.2 (138edd61fd100ec658bfa2d307c43b76940a5d7d; 2017-10-18T07:58:13Z) |
Default Java | 1.8.0_151 |
rubocop | v0.52.1 |
rubocop | https://builds.apache.org/job/PreCommit-HBASE-Build/11055/artifact/patchprocess/diff-patch-rubocop.txt |
ruby-lint | v2.3.1 |
ruby-lint | https://builds.apache.org/job/PreCommit-HBASE-Build/11055/artifact/patchprocess/diff-patch-ruby-lint.txt |
Test Results | https://builds.apache.org/job/PreCommit-HBASE-Build/11055/testReport/ |
modules | C: hbase-shell . U: . |
Console output | https://builds.apache.org/job/PreCommit-HBASE-Build/11055/console |
Powered by | Apache Yetus 0.6.0 http://yetus.apache.org |
This message was automatically generated.
A bit confused by how the problem relates to the solution here... Can you give it another go at explaining please, elserj? The problem of not having exit status seems clear enough to solve on it's own, but I'm not connecting the dots on how this relates to array passing.
Also, want to make sure I understand the impact - we currently have scripts that use -n and check the exit status. Will they need to be updated to use -r as well? I think non-interactive mode does always want it, and interactive mode can be a toggle, that's fine.
Thanks, Mike. No worries on another explanation.
Our appy introduced the interactive flag to the shell. "Interactive" mode was meant to be designed for when a user is sitting in front of the terminal typing in commands, and "non-interactive" was for when a file of commands are provided. Non-interactive mode did a few things like:
- Shell exit on first failed command in the file of commands
- Different I/O setup to IRB
- Values returned by command methods
The opposite of each of these are true for the interactive mode:
- Shell does not exit for any failed command
- Commands' return values are not given to the user
My argument is that the latter should still have the ability to "opt-in". For example, power-users know that they're in a ruby shell and would want to do fun things like: list.each{|t| disable t; drop t}. I introduced the -r option that, regardless of the interactive/non-interactive mode, would cause shell commands to return values to the user. The default functionality has not changed.
Looking at the patch in other jira (HBASE-15965) again, looks like the gist was to make the behavior of shell commands more consistent. Some commands printed output and returned results (some printed the results too as duplicate, some didn't), some commands didn't return any result but just printed something, etc.
It's a mess to maintain code when exceptions are norm rather than rules, what was being suggested as alternative in that jira (i feel bad that i missed to reply it though)
That said, if there's a suggestion to improve shell experience in a way more than - print lists for some selected commands, all up for it.
Jira comments in my window were out-of-date. Your last comment makes perfect sense elserj. +1
looks like the gist was to make the behavior of shell commands more consistent. Some commands printed output and returned results (some printed the results too as duplicate, some didn't), some commands didn't return any result but just printed something, etc.
Yup, totally understand why the change was made. I think, for normal users, this is fine. That's why I've introduced this flag (default: false) to let those of us who know what we're doing to keep doing things how we've been doing them.
That said, if there's a suggestion to improve shell experience in a way more than - print lists for some selected commands, all up for it.
It would be nice if we could just control this at the IRB level, but I haven't looked to see if it's possible to suppress the shell printing the values.
Fix versions for HBASE-15965 was 1.4.0 and 2.0.0, so we should make this change in branch-1, branch-2, and master at least. Maybe in 1.4.x too?
The change is not enabling return-values by default when shell is started in non-interactive mode. Currently, 'n' is useless without '-r'. Shouldn't we set return_value=true for "-noninteractive" arg case too?
nit: reverse the condition?
+ if not return_values return nil else return ret
we should make this change in branch-1, branch-2, and master at least. Maybe in 1.4.x too?
Fine for me too. Didn't spend any time thinking about where all it should land.
The change is not enabling return-values by default when shell is started in non-interactive mode.
Ahh, good catch. I forgot to do that. Agree that it should only be relevant for interactive mode.
Interactive | Return Values |
---|---|
True | False (default) |
False | True (default) |
nit: reverse the condition?
Sure . I had this as unless return_values earlier and some of the ruby checkstyle things yelled at me. Didn't reverse it entirely
btw, you don't need the return keyword and you especially don't need return nil.
Assuming, I got the condition correctly, this should work for you:
def command(command, *args)
ret = internal_command(command, :command, *args)
ret if return_values or not interactive
end
Just attached .003 which sets a better default value for return_values and implements Mike's ruby-style suggestion.
gave +1 earlier, feel free to submit whenever you're done with minor changes afa i'm concerned. (just stating it explicitly )
The change is not enabling return-values by default when shell is started in non-interactive mode.
Ahh, good catch. I forgot to do that. Agree that it should only be relevant for interactive mode.
V3 still didn't address this I think?
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 9s | Docker mode activated. |
Prechecks | |||
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
-1 | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
branch-2 Compile Tests | |||
0 | mvndep | 58m 40s | Maven dependency ordering for branch |
+1 | mvninstall | 98m 19s | branch-2 passed |
+1 | javadoc | 3m 52s | branch-2 passed |
Patch Compile Tests | |||
0 | mvndep | 0m 9s | Maven dependency ordering for patch |
+1 | mvninstall | 3m 37s | the patch passed |
-1 | rubocop | 0m 6s | The patch generated 3 new + 48 unchanged - 2 fixed = 51 total (was 50) |
+1 | ruby-lint | 0m 3s | There were no new ruby-lint issues. |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | javadoc | 2m 34s | the patch passed |
Other Tests | |||
-1 | unit | 112m 50s | root in the patch failed. |
+1 | asflicense | 0m 36s | The patch does not generate ASF License warnings. |
281m 15s |
Reason | Tests |
---|---|
Failed junit tests | hadoop.hbase.snapshot.TestRegionSnapshotTask |
Subsystem | Report/Notes |
---|---|
Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hbase:9f2f2db |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12905736/HBASE-19770.001.branch-2.patch |
Optional Tests | asflicense rubocop ruby_lint javac javadoc unit |
uname | Linux 720c4193d002 3.13.0-135-generic #184-Ubuntu SMP Wed Oct 18 11:55:51 UTC 2017 x86_64 GNU/Linux |
Build tool | maven |
Personality | /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/component/dev-support/hbase-personality.sh |
git revision | branch-2 / 20afaca769 |
maven | version: Apache Maven 3.5.2 (138edd61fd100ec658bfa2d307c43b76940a5d7d; 2017-10-18T07:58:13Z) |
Default Java | 1.8.0_151 |
rubocop | v0.52.1 |
rubocop | https://builds.apache.org/job/PreCommit-HBASE-Build/11077/artifact/patchprocess/diff-patch-rubocop.txt |
ruby-lint | v2.3.1 |
unit | https://builds.apache.org/job/PreCommit-HBASE-Build/11077/artifact/patchprocess/patch-unit-root.txt |
Test Results | https://builds.apache.org/job/PreCommit-HBASE-Build/11077/testReport/ |
modules | C: hbase-shell . U: . |
Console output | https://builds.apache.org/job/PreCommit-HBASE-Build/11077/console |
Powered by | Apache Yetus 0.6.0 http://yetus.apache.org |
This message was automatically generated.
V3 still didn't address this I think?
Hah, and that's why I put up another patch
gave +1 earlier, feel free to submit whenever you're done with minor changes afa i'm concerned. (just stating it explicitly )
Ok, thanks again, Appy! I have Mike's changes done locally. Will push them up assuming the shell tests pass on master (branch-2 are good).
andrew.purtell@gmail.com, you OK with this change for branch-1.4?
.004 The patch I had committed to branch-2 and master, but Mike pointed out that I should have used unless instead of the if not I did use.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | Docker mode activated. |
-1 | patch | 0m 5s | |
Subsystem | Report/Notes |
---|---|
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12905736/HBASE-19770.001.branch-2.patch |
Console output | https://builds.apache.org/job/PreCommit-HBASE-Build/11087/console |
Powered by | Apache Yetus 0.6.0 http://yetus.apache.org |
This message was automatically generated.
FAILURE: Integrated in Jenkins build HBase-Trunk_matrix #4420 (See https://builds.apache.org/job/HBase-Trunk_matrix/4420/)
HBASE-19770 Separate command return values from interactive shells (elserj: rev 7224546b1ed99faf41ecef82043971c7d44a5836)
- (edit) hbase-shell/src/main/ruby/shell.rb
- (edit) bin/hirb.rb
- (edit) hbase-shell/src/test/ruby/test_helper.rb
- (edit) hbase-shell/src/test/ruby/shell/noninteractive_test.rb
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | Docker mode activated. |
-1 | patch | 0m 5s | |
Subsystem | Report/Notes |
---|---|
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12905736/HBASE-19770.001.branch-2.patch |
Console output | https://builds.apache.org/job/PreCommit-HBASE-Build/11113/console |
Powered by | Apache Yetus 0.6.0 http://yetus.apache.org |
This message was automatically generated.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 9s | Docker mode activated. |
Prechecks | |||
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
-1 | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
branch-2 Compile Tests | |||
+1 | mvninstall | 3m 47s | branch-2 passed |
+1 | javadoc | 0m 10s | branch-2 passed |
Patch Compile Tests | |||
+1 | mvninstall | 3m 44s | the patch passed |
+1 | rubocop | 0m 5s | The patch generated 0 new + 27 unchanged - 2 fixed = 27 total (was 29) |
+1 | ruby-lint | 0m 2s | There were no new ruby-lint issues. |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | javadoc | 0m 10s | the patch passed |
Other Tests | |||
+1 | unit | 7m 11s | hbase-shell in the patch passed. |
+1 | asflicense | 0m 10s | The patch does not generate ASF License warnings. |
15m 40s |
Subsystem | Report/Notes |
---|---|
Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hbase:9f2f2db |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12906863/HBASE-19770.000.branch-2.addendum.patch |
Optional Tests | asflicense javac javadoc unit rubocop ruby_lint |
uname | Linux 2355de093e39 3.13.0-139-generic #188-Ubuntu SMP Tue Jan 9 14:43:09 UTC 2018 x86_64 GNU/Linux |
Build tool | maven |
Personality | /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build@2/component/dev-support/hbase-personality.sh |
git revision | branch-2 / c01dc69123 |
maven | version: Apache Maven 3.5.2 (138edd61fd100ec658bfa2d307c43b76940a5d7d; 2017-10-18T07:58:13Z) |
Default Java | 1.8.0_151 |
rubocop | v0.52.1 |
ruby-lint | v2.3.1 |
Test Results | https://builds.apache.org/job/PreCommit-HBASE-Build/11131/testReport/ |
modules | C: hbase-shell U: hbase-shell |
Console output | https://builds.apache.org/job/PreCommit-HBASE-Build/11131/console |
Powered by | Apache Yetus 0.6.0 http://yetus.apache.org |
This message was automatically generated.
FAILURE: Integrated in Jenkins build HBase-Trunk_matrix #4462 (See https://builds.apache.org/job/HBase-Trunk_matrix/4462/)
HBASE-19770 ADDENDUM Replace `if not` with `unless` (josh.elser: rev 8b2b59fdeefba57ee49d225b26fcd37f5cfa55b2)
- (edit) hbase-shell/src/main/ruby/shell.rb
Sorry I missed the at-mention. I see this neither committed to branch-1.4 nor branch-1. No issues at all with commit to either. Would certainly be fine to put it in branch-1 for 1.5.
Thanks, Andrew! Will do so (lopping the addendum in to make one commit on those).
Was trying the example in HBASE-20151 and realized that our users may be relying on return values more than we thought. Things worked when i started shell with -r.
Just noting it....and that it'd been best if there was some way to tell jruby shell to simply ignore the returned value if there was no assignment.
appy, thanks for the feedback. It was definitely something that caught us by surprise at $dayjob. We simply reverted the behavior entirely for our testing framework.
I think flipping the default to true is something we could consider if this "experiment" is deemed not as helpful as intended.
I think flipping the default to true is something we could consider if this "experiment" is deemed not as helpful as intended.
sounds right.
Pardon for starting down this road.
wdys? default return-value to true (And rename option to --no-return-value) or remove the option altogether?
No worries, Appy! You were trying to make things better – I'll never hold a grudge over that.
wdys? default return-value to true (And rename option to --no-return-value) or remove the option altogether?
I would say we just flip the default from false to true.
What we noticed with meszibalu is when you run the shell without --return-values you can't use get_table command to assign the table to a variable. Because of this I'd also like to use return-values as default or at least document it in the help message that users must use -r to call get_table.
without --return-values you can't use get_table command to assign the table to a variable
Eek. That's a bad one. Let me open up another issue and flip the default instead of mulling it over more.
Looks like this one is caused by
HBASE-15965. FYI appy.I can understand wanting to prevent IRB from printing the object from the command (when the formatter printed it nicely), but are we sure we want to do this by default? Caught me by surprise. I'm not sure I'd agree with the assertion that "all interactive shells do not want return values".