[jira] [Commented] (HBASE-15965) Shell test changes. Use @shell.command instead directly calling functions in admin.rb and other libraries.

["Junegunn Choi (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HBASE-15965?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15358223#comment-15358223 ] 

Junegunn Choi commented on HBASE-15965:
---------------------------------------

[~Appy]
I was reminded of the same xkcd strip when I first ran into this :)

bq. You should be able to get the return value using -n (non-interactive) flag.

Yes, but it's quite useful to be able to *incrementally explore* the data interactively on the REPL. Internally, we've been using a customized version of the shell with a few extra commands that allow us to explore the status of the cluster without having to write verbose Java code with Admin API. One such command is {{regions}}, and this is an example of what we do when things happen.

{code}
# See the number of regions
regions.count

# See the number of regions whose localities are below 50%
regions.select { |r| r[:locality] < 50 }

# Pretty-print those regions grouped by their tables
require 'pp'
pp regions.select { |r| r[:locality] < 50 }.group_by { |r| r[:table] }

# Flush and major compact regions that match the criteria
regions.select { |r| r[:locality] < 50 && r[:files] > 2 }.each { |r| flush r[:name]; major_compact r[:name] }
{code}

If I had to use {{-n}} flag, I'll have to re-run hbase shell multiple times. There's nothing wrong with harnessing the power of REPL.

bq. Changing return value to contain more information will break someone else's workflow

Yes, the concern led me to the idea of introducing new commands instead of changing the return values of the existing ones.

bq. That approach requires duplicating classes and we over 100 commands

I think we have a misunderstanding here. I was not suggesting that we should replicate every one of the commands, but to add just a handful of new ones (tables, snapshots, regions, servers, etc.) under a new command group named something like QUERY or INSPECTION whose members are supposed to return values with clear semantics. They aren't for side-effects, they are not responsible for formatting and printing the output, they should just return values that can be used by the user programmatically even in "interactive" mode. I don't disagree with suppressing the return values of the commands in other groups for cleaner output, but the commands in this group are solely for their return values, so we make an exception. What do you think?

> Shell test changes. Use @shell.command instead directly calling functions in admin.rb and other libraries.
> ----------------------------------------------------------------------------------------------------------
>
>                 Key: HBASE-15965
>                 URL: https://issues.apache.org/jira/browse/HBASE-15965
>             Project: HBase
>          Issue Type: Bug
>            Reporter: Appy
>            Assignee: Appy
>             Fix For: 2.0.0
>
>         Attachments: HBASE-15965.master.001.patch, HBASE-15965.master.002.patch, HBASE-15965.master.003.patch
>
>
> Testing by executing a command will cover the exact path users will trigger, so its better then directly calling library functions in tests. Changing the tests to use @shell.command(:<command>, args) to execute them like it's a command coming from shell.
> Norm change:
> Commands should print the output user would like to see, but in the end, should also return the relevant value. This way:
> - Tests can use returned value to check that functionality works
> - Tests can capture stdout to assert particular kind of output user should see.
> - We do not print the return value in interactive mode and keep the output clean. See Shell.command() function.
> Bugs found due to this change:
> - Uncovered bug in major_compact.rb with this approach. It was calling admin.majorCompact() which doesn't exist but our tests didn't catch it since they directly tested admin.major_compact()
> - Enabled TestReplicationShell. If it's bad, flaky infra will take care of it.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)