hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Junegunn Choi (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-15965) Shell test changes. Use @shell.command instead directly calling functions in admin.rb and other libraries.
Date Fri, 01 Jul 2016 02:00:16 GMT

    [ 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:

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.

# See the number of regions

# 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] }

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

View raw message