Skip to content

HBASE-26543 correct parsing of shell args with GetoptLong #4000

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

HBASE-26543 correct parsing of shell args with GetoptLong #4000

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
965d830
HBASE-26543 correct parsing of shell args with GetoptLong
busbey Dec 15, 2021
6b70d50
HBASE-26543 correct issues brought up by Rubocop.
busbey Jan 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 33 additions & 36 deletions hbase-shell/src/main/ruby/jar-bootstrap.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,62 +78,59 @@ def add_to_configuration(c, arg)
c
end

conf_from_cli = nil

# strip out any config definitions that won't work with GetoptLong
D_ARG = '-D'.freeze
ARGV.delete_if do |arg|
if arg.start_with?(D_ARG) && arg.include?('=')
conf_from_cli = add_to_configuration(conf_from_cli, arg[2..-1])
true
else
false
end
end

opts = GetoptLong.new(
[ '--help', '-h', GetoptLong::NO_ARGUMENT ],
[ '--debug', '-d', GetoptLong::OPTIONAL_ARGUMENT ],
[ '--noninteractive', '-n', GetoptLong::OPTIONAL_ARGUMENT ],
[ '--top-level-defs', GetoptLong::OPTIONAL_ARGUMENT ],
[ '--Dkey=value', '-D', GetoptLong::NO_ARGUMENT ]
['--help', '-h', GetoptLong::NO_ARGUMENT],
['--debug', '-d', GetoptLong::NO_ARGUMENT],
['--noninteractive', '-n', GetoptLong::NO_ARGUMENT],
['--top-level-defs', GetoptLong::NO_ARGUMENT],
['-D', GetoptLong::REQUIRED_ARGUMENT],
['--return-values', '-r', GetoptLong::NO_ARGUMENT]
)
opts.ordering = GetoptLong::REQUIRE_ORDER
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this going to break back compat somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, this ordering option should bring us closer to before HBASE-24772 was introduced. I reasonably believe it is compatible with our parsing prior to that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for those interested in more details here are the docs for Ruby GetoptLong's ordering:

https://ruby-doc.org/stdlib-3.0.3/libdoc/getoptlong/rdoc/GetoptLong.html#method-i-ordering-3D

specifically, this patch expressly has us use REQUIRE_ORDER:

REQUIRE_ORDER :

Options are required to occur before non-options.

Processing of options ends as soon as a word is encountered that has not been preceded by an appropriate option flag.

I believe this matches the behavior of our manual cli argument parsing prior to HBASE-24772.

Note that by default GetoptLong picks an ordering based on an environment variable we don't set. The default in that case is PERMUTE :

PERMUTE :

Options can occur anywhere in the command line parsed. This is the default behavior.

Every sequence of words which can be interpreted as an option (with or without argument) is treated as an option; non-option words are skipped.

This ordering option is why after HBASE-24772, and prior to this patch, passing in IRB's "ignore .irbrc" flag ( -f) would cause our shell to crash with an "I don't know what that option is supposed to mean".


found = []
script2run = nil
log_level = org.apache.log4j.Level::ERROR
@shell_debug = false
interactive = true
top_level_definitions = false
_configuration = nil
D_ARG = '-D'.freeze

opts.each do |opt, arg|
case opt || arg
when '--help' || '-h'
case opt
when '--help'
puts cmdline_help
exit
when D_ARG
argValue = ARGV.shift || (raise "#{D_ARG} takes a 'key=value' parameter")
_configuration = add_to_configuration(_configuration, argValue)
found.push(arg)
found.push(argValue)
when arg.start_with?(D_ARG)
_configuration = add_to_configuration(_configuration, arg[2..-1])
found.push(arg)
when '--debug'|| '-d'
conf_from_cli = add_to_configuration(conf_from_cli, arg)
when '--debug'
log_level = org.apache.log4j.Level::DEBUG
$fullBackTrace = true
@shell_debug = true
found.push(arg)
puts 'Setting DEBUG log level...'
when '--noninteractive'|| '-n'
interactive = false
found.push(arg)
when '--return-values' || 'r'
warn '[INFO] the -r | --return-values option is ignored. we always behave '\
when '--noninteractive'
interactive = false
when '--return-values'
warn '[INFO] the -r | --return-values option is ignored. we always behave '\
'as though it was given.'
found.push(arg)
when '--top-level-defs'
top_level_definitions = true
else
# Presume it a script. Save it off for running later below
# after we've set up some environment.
script2run = arg
found.push(arg)
# Presume that any other args are meant for the script.
when '--top-level-defs'
top_level_definitions = true
end
end

script2run = ARGV.shift unless ARGV.empty?

# Delete all processed args
found.each { |arg| ARGV.delete(arg) }
# Make sure debug flag gets back to IRB
ARGV.unshift('-d') if @shell_debug

Expand All @@ -151,7 +148,7 @@ def add_to_configuration(c, arg)
require 'shell/formatter'

# Setup the HBase module. Create a configuration.
@hbase = _configuration.nil? ? Hbase::Hbase.new : Hbase::Hbase.new(_configuration)
@hbase = conf_from_cli.nil? ? Hbase::Hbase.new : Hbase::Hbase.new(conf_from_cli)

# Setup console
@shell = Shell::Shell.new(@hbase, interactive)
Expand Down