Uploaded image for project: 'HBase'
  1. HBase
  2. HBASE-19770

Add '--return-values' option to Shell to print return values of commands in interactive mode

Details

    • Improvement
    • Status: Closed
    • Critical
    • Resolution: Fixed
    • None
    • 2.0.0-beta-2, 1.4.2, 2.0.0
    • shell
    • None
    • Reviewed
    • Hide
      Introduces 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.
      Show
      Introduces 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

        1. HBASE-19770.000.branch-2.addendum.patch
          0.8 kB
          Josh Elser
        2. HBASE-19770.001.branch-2.patch
          3 kB
          Josh Elser
        3. HBASE-19770.002.branch-2.patch
          4 kB
          Josh Elser
        4. HBASE-19770.003.branch-2.patch
          5 kB
          Josh Elser
        5. HBASE-19770.004.branch-2.patch
          5 kB
          Josh Elser
        6. HBASE-19770.ADDENDUM.branch-2.patch
          0.8 kB
          Josh Elser

        Issue Links

          Activity

            elserj Josh Elser added a comment -

            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".

            elserj Josh Elser added a comment - 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".
            elserj Josh Elser added a comment -

            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."

            elserj Josh Elser added a comment - 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."
            elserj Josh Elser added a comment -

            .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...

            elserj Josh Elser added a comment - .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...
            hadoopqa Hadoop QA added a comment -
            -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 HBASE-19770
            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.

            hadoopqa Hadoop QA added a comment - -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 HBASE-19770 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.
            elserj Josh Elser added a comment -

            .002 Fix ruby "style" issues. Make sure return_values=true for the ruby tests too (root-cause of test failures from .001)

            elserj Josh Elser added a comment - .002 Fix ruby "style" issues. Make sure return_values=true for the ruby tests too (root-cause of test failures from .001)
            hadoopqa Hadoop QA added a comment -
            -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 HBASE-19770
            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.

            hadoopqa Hadoop QA added a comment - -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 HBASE-19770 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.
            elserj Josh Elser added a comment -

            mdrob, do you have time for a quick review here?

            elserj Josh Elser added a comment - mdrob , do you have time for a quick review here?
            mdrob Mike Drob added a comment -

            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.

            mdrob Mike Drob added a comment - 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.
            elserj Josh Elser added a comment - - edited

            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.

            elserj Josh Elser added a comment - - edited 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.

            appy Apekshit Sharma added a comment - 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

            appy Apekshit Sharma added a comment - Jira comments in my window were out-of-date. Your last comment makes perfect sense elserj . +1
            elserj Josh Elser added a comment -

            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.

            elserj Josh Elser added a comment - 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
            
            appy Apekshit Sharma added a comment - 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
            elserj Josh Elser added a comment -

            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

            elserj Josh Elser added a comment - 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
            mdrob Mike Drob added a comment -

            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
            
            mdrob Mike Drob added a comment - 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
            elserj Josh Elser added a comment -

            Just attached .003 which sets a better default value for return_values and implements Mike's ruby-style suggestion.

            elserj Josh Elser added a comment - 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 )

            appy Apekshit Sharma added a comment - gave +1 earlier, feel free to submit whenever you're done with minor changes afa i'm concerned. (just stating it explicitly )
            mdrob Mike Drob added a comment -

            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?

            mdrob Mike Drob added a comment - 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?
            hadoopqa Hadoop QA added a comment -
            -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 HBASE-19770
            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.

            hadoopqa Hadoop QA added a comment - -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 HBASE-19770 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.
            elserj Josh Elser added a comment -

            V3 still didn't address this I think?

            Hah, and that's why I put up another patch

            elserj Josh Elser added a comment - V3 still didn't address this I think? Hah, and that's why I put up another patch
            elserj Josh Elser added a comment -

            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?

            elserj Josh Elser added a comment - 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?
            elserj Josh Elser added a comment -

            .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.

            elserj Josh Elser added a comment - .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.
            hadoopqa Hadoop QA added a comment -
            -1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 0s Docker mode activated.
            -1 patch 0m 5s HBASE-19770 does not apply to branch-2. Rebase required? Wrong Branch? See https://yetus.apache.org/documentation/0.6.0/precommit-patchnames for help.



            This message was automatically generated.

            hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. -1 patch 0m 5s HBASE-19770 does not apply to branch-2. Rebase required? Wrong Branch? See https://yetus.apache.org/documentation/0.6.0/precommit-patchnames for help. Subsystem Report/Notes JIRA Issue HBASE-19770 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.
            hudson Hudson added a comment -

            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
            hudson Hudson added a comment - 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
            elserj Josh Elser added a comment -

            mdrob, addendum patch posted for the if not -> unless

            elserj Josh Elser added a comment - mdrob , addendum patch posted for the if not -> unless
            hadoopqa Hadoop QA added a comment -
            -1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 0s Docker mode activated.
            -1 patch 0m 5s HBASE-19770 does not apply to branch-2. Rebase required? Wrong Branch? See https://yetus.apache.org/documentation/0.6.0/precommit-patchnames for help.



            This message was automatically generated.

            hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. -1 patch 0m 5s HBASE-19770 does not apply to branch-2. Rebase required? Wrong Branch? See https://yetus.apache.org/documentation/0.6.0/precommit-patchnames for help. Subsystem Report/Notes JIRA Issue HBASE-19770 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.
            hadoopqa Hadoop QA added a comment -
            -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 HBASE-19770
            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.

            hadoopqa Hadoop QA added a comment - -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 HBASE-19770 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.
            elserj Josh Elser added a comment -

            mdrob, you good on this addendum?

            elserj Josh Elser added a comment - mdrob , you good on this addendum?
            mdrob Mike Drob added a comment -

            Yep, missed it earlier, thanks Josh

            mdrob Mike Drob added a comment - Yep, missed it earlier, thanks Josh
            elserj Josh Elser added a comment -

            Addendum for the rubystyle pushed.

            elserj Josh Elser added a comment - Addendum for the rubystyle pushed.
            hudson Hudson added a comment -

            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
            hudson Hudson added a comment - 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.

            apurtell Andrew Kyle Purtell added a comment - 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.
            elserj Josh Elser added a comment -

            Thanks, Andrew! Will do so (lopping the addendum in to make one commit on those).

            elserj Josh Elser added a comment - 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 Apekshit Sharma added a comment - 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.
            elserj Josh Elser added a comment -

            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.

            elserj Josh Elser added a comment - 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.

            appy Apekshit Sharma added a comment - 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?

            appy Apekshit Sharma added a comment - wdys? default return-value to true (And rename option to --no-return-value) or remove the option altogether?
            elserj Josh Elser added a comment -

            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.

            elserj Josh Elser added a comment - 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 .
            psomogyi Peter Somogyi added a comment - - edited

            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.

            psomogyi Peter Somogyi added a comment - - edited 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.
            elserj Josh Elser added a comment -

            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.

            elserj Josh Elser added a comment - 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.

            People

              elserj Josh Elser
              romil.choksi Romil Choksi
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: