From da88f0bc7bf32bf5e78a18250c05c44b0ef60650 Mon Sep 17 00:00:00 2001 From: takkanm Date: Thu, 6 Aug 2015 19:02:12 +0900 Subject: [PATCH 01/10] connector:issue recommend use arugument now `--database` and `--table` is obsolete --- lib/td/command/connector.rb | 37 +++++++++++++++++++++--------- lib/td/command/list.rb | 2 +- spec/td/command/connector_spec.rb | 38 +++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 12 deletions(-) diff --git a/lib/td/command/connector.rb b/lib/td/command/connector.rb index de7c66ec..78957469 100644 --- a/lib/td/command/connector.rb +++ b/lib/td/command/connector.rb @@ -104,17 +104,23 @@ def connector_preview(op) $stdout.puts cmd_render_table(rows, :fields => fields, :render_format => op.render_format, :resize => false) $stdout.puts "Update #{config_file} and use '#{$prog} " + Config.cl_options_string + "connector:preview #{config_file}' to preview again." - $stdout.puts "Use '#{$prog} " + Config.cl_options_string + "connector:issue #{config_file}' to run Server-side bulk load." + $stdout.puts "Use '#{$prog} " + Config.cl_options_string + "connector:issue database_name table_name #{config_file}' to run Server-side bulk load." end def connector_issue(op) - database = table = nil - time_column = nil - wait = exclude = false - auto_create = false - - op.on('--database DB_NAME', "destination database") { |s| database = s } - op.on('--table TABLE_NAME', "destination table") { |s| table = s } + option_database = option_table = nil + time_column = nil + wait = exclude = false + auto_create = false + + op.on('--database DB_NAME', "(obsoleted)") { |s| + $stderr.puts '--database is obsolete option' + option_database = s + } + op.on('--table TABLE_NAME', "(obsoleted)") { |s| + $stderr.puts '--table is obsolete option' + option_table = s + } op.on('--time-column COLUMN_NAME', "data partitioning key") { |s| time_column = s } # unnecessary but for backward compatibility op.on('-w', '--wait', 'wait for finishing the job', TrueClass) { |b| wait = b } op.on('-x', '--exclude', 'do not automatically retrieve the job result', TrueClass) { |b| exclude = b } @@ -122,10 +128,19 @@ def connector_issue(op) auto_create = b } - config_file = op.cmd_parse + database, table, config_file = nil + args = op.cmd_parse + + if args.instance_of? String + config_file = args + database = option_database + table = option_table + else + database, table, config_file = args + end - required('--database', database) - required('--table', table) + required('database', database) + required('table', table) config = prepare_bulkload_job_config(config_file) (config['out'] ||= {})['time_column'] = time_column if time_column # TODO will not work once embulk implements multi-job diff --git a/lib/td/command/list.rb b/lib/td/command/list.rb index f98fc17c..db64f10e 100644 --- a/lib/td/command/list.rb +++ b/lib/td/command/list.rb @@ -345,7 +345,7 @@ def self.finishup add_list 'connector:guess', %w[config?], 'Run guess to generate connector config file', ["connector:guess config.yml -o td-bulkload.yml\n\nexample config.yml:#{connector_guess_example_config}"] add_list 'connector:preview', %w[config], 'Show preview of connector execution', ['connector:preview td-bulkload.yml'] - add_list 'connector:issue', %w[config], 'Run one time connector execution', ['connector:issue td-bulkload.yml'] + add_list 'connector:issue', %w[config database table], 'Run one time connector execution', ['connector:issue example_db event_logs td-bulkload.yml'] add_list 'connector:list', %w[], 'Show list of connector sessions', ['connector:list'] add_list 'connector:create', %w[name cron database table config], 'Create new connector session', ['connector:create connector1 "0 * * * *" connector_database connector_table td-bulkload.yml'] diff --git a/spec/td/command/connector_spec.rb b/spec/td/command/connector_spec.rb index bd63c83d..a73b795d 100644 --- a/spec/td/command/connector_spec.rb +++ b/spec/td/command/connector_spec.rb @@ -80,6 +80,44 @@ module TreasureData::Command end end + describe 'database and table arguments' do + let(:database) { 'database' } + let(:table) { 'table' } + let(:config) { {} } + + before do + client = double(:client) + command.stub(:get_client).and_return(client) + command.stub(:create_database_and_table_if_not_exist) + command.stub(:prepare_bulkload_job_config).and_return(config) + client.should_receive(:bulk_load_issue).with(database, table, {config: config}).and_return(1234) + + subject + end + + context 'set --database and --table' do + let(:option) { + List::CommandParser.new("connector:issue", ["config"], ['database', 'table'], nil, [File.join("spec", "td", "fixture", "bulk_load.yml"), '--database', database, '--table', table], true) + } + + it 'show warning' do + expect(stderr_io.string).to include '--database is obsolete option' + expect(stderr_io.string).to include '--table is obsolete option' + end + end + + context 'set arguments' do + let(:option) { + List::CommandParser.new("connector:issue", ["config", 'database', 'table'], [], nil, [database, table, File.join("spec", "td", "fixture", "bulk_load.yml")], true) + } + + it 'no warning' do + expect(stderr_io.string).not_to include '--database is obsolete option' + expect(stderr_io.string).not_to include '--table is obsolete option' + end + end + end + describe 'queueing job' do let(:option) { List::CommandParser.new("connector:issue", ["config"], ['database', 'table'], nil, [File.join("spec", "td", "fixture", "bulk_load.yml"), '--database', 'database', '--table', 'table'], true) From 41edc8cfd1c5da847945f4639207757ea3cf5ced Mon Sep 17 00:00:00 2001 From: takkanm Date: Thu, 20 Aug 2015 14:43:28 +0900 Subject: [PATCH 02/10] fix argument setting --- lib/td/command/list.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/td/command/list.rb b/lib/td/command/list.rb index db64f10e..8ffa6a87 100644 --- a/lib/td/command/list.rb +++ b/lib/td/command/list.rb @@ -345,7 +345,7 @@ def self.finishup add_list 'connector:guess', %w[config?], 'Run guess to generate connector config file', ["connector:guess config.yml -o td-bulkload.yml\n\nexample config.yml:#{connector_guess_example_config}"] add_list 'connector:preview', %w[config], 'Show preview of connector execution', ['connector:preview td-bulkload.yml'] - add_list 'connector:issue', %w[config database table], 'Run one time connector execution', ['connector:issue example_db event_logs td-bulkload.yml'] + add_list 'connector:issue', %w[config], 'Run one time connector execution', ['connector:issue example_db event_logs td-bulkload.yml'] add_list 'connector:list', %w[], 'Show list of connector sessions', ['connector:list'] add_list 'connector:create', %w[name cron database table config], 'Create new connector session', ['connector:create connector1 "0 * * * *" connector_database connector_table td-bulkload.yml'] From 251e99270b438bc3e99e1080a20955c5b1a43070 Mon Sep 17 00:00:00 2001 From: takkanm Date: Thu, 20 Aug 2015 15:26:53 +0900 Subject: [PATCH 03/10] refactor spec --- spec/td/command/connector_spec.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/spec/td/command/connector_spec.rb b/spec/td/command/connector_spec.rb index a73b795d..ac9d4620 100644 --- a/spec/td/command/connector_spec.rb +++ b/spec/td/command/connector_spec.rb @@ -90,7 +90,7 @@ module TreasureData::Command command.stub(:get_client).and_return(client) command.stub(:create_database_and_table_if_not_exist) command.stub(:prepare_bulkload_job_config).and_return(config) - client.should_receive(:bulk_load_issue).with(database, table, {config: config}).and_return(1234) + client.should_receive(:bulk_load_issue).with(database, table, {config: expect_config}).and_return(1234) subject end @@ -99,6 +99,9 @@ module TreasureData::Command let(:option) { List::CommandParser.new("connector:issue", ["config"], ['database', 'table'], nil, [File.join("spec", "td", "fixture", "bulk_load.yml"), '--database', database, '--table', table], true) } + let(:expect_config) { + {'out' => {'database' => database, 'table' => table}} + } it 'show warning' do expect(stderr_io.string).to include '--database is obsolete option' @@ -110,6 +113,9 @@ module TreasureData::Command let(:option) { List::CommandParser.new("connector:issue", ["config", 'database', 'table'], [], nil, [database, table, File.join("spec", "td", "fixture", "bulk_load.yml")], true) } + let(:expect_config) { + {'out' => {'database' => database, 'table' => table}} + } it 'no warning' do expect(stderr_io.string).not_to include '--database is obsolete option' From dce17780f833a685de20a05f95669c26ff4efae9 Mon Sep 17 00:00:00 2001 From: takkanm Date: Thu, 20 Aug 2015 15:53:23 +0900 Subject: [PATCH 04/10] use and set config file value --- lib/td/command/connector.rb | 21 +++++++++++++------ spec/td/command/connector_spec.rb | 35 +++++++++++++++++++++++++++++-- 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/lib/td/command/connector.rb b/lib/td/command/connector.rb index 78957469..f7414829 100644 --- a/lib/td/command/connector.rb +++ b/lib/td/command/connector.rb @@ -139,19 +139,19 @@ def connector_issue(op) database, table, config_file = args end - required('database', database) - required('table', table) - config = prepare_bulkload_job_config(config_file) - (config['out'] ||= {})['time_column'] = time_column if time_column # TODO will not work once embulk implements multi-job + overwrite_out_config(config, 'database' => database, 'table' => table, 'time_column' => time_column) client = get_client() + required('database', config['out']['database']) + required('table', config['out']['table']) + if auto_create - create_database_and_table_if_not_exist(client, database, table) + create_database_and_table_if_not_exist(client, config['out']['database'], config['out']['table']) end - job_id = client.bulk_load_issue(database, table, config: config) + job_id = client.bulk_load_issue(config['out']['database'], config['out']['table'], config: config) $stdout.puts "Job #{job_id} is queued." $stdout.puts "Use '#{$prog} " + Config.cl_options_string + "job:show #{job_id}' to show the status." @@ -161,6 +161,15 @@ def connector_issue(op) end end + def overwrite_out_config(config, out_args) + # TODO will not work once embulk implements multi-job + config['out'] ||= {} + + out_args.each do |key, value| + config['out'][key] = value if value + end + end + def connector_list(op) set_render_format_option(op) op.cmd_parse diff --git a/spec/td/command/connector_spec.rb b/spec/td/command/connector_spec.rb index ac9d4620..c1169f1c 100644 --- a/spec/td/command/connector_spec.rb +++ b/spec/td/command/connector_spec.rb @@ -83,7 +83,6 @@ module TreasureData::Command describe 'database and table arguments' do let(:database) { 'database' } let(:table) { 'table' } - let(:config) { {} } before do client = double(:client) @@ -91,19 +90,48 @@ module TreasureData::Command command.stub(:create_database_and_table_if_not_exist) command.stub(:prepare_bulkload_job_config).and_return(config) client.should_receive(:bulk_load_issue).with(database, table, {config: expect_config}).and_return(1234) + end + + context 'set from config file' do + let(:expect_config) { + {'out' => {'database' => database, 'table' => table}} + } + + context 'without arguments' do + let(:option) { + List::CommandParser.new("connector:issue", ["config"], [], nil, [File.join("spec", "td", "fixture", "bulk_load.yml")], true) + } + let(:config) { + {'out' => {'database' => database, 'table' => table}} + } + + it { expect { subject }.not_to raise_error } + end + + context 'with arguments' do + let(:option) { + List::CommandParser.new("connector:issue", ["config"], ['database', 'table'], nil, [File.join("spec", "td", "fixture", "bulk_load.yml"), '--database', database, '--table', table], true) + } + let(:config) { + {'out' => {'database' => 'config_database', 'table' => 'config_table'}} + } - subject + it { expect { subject }.not_to raise_error } + end end context 'set --database and --table' do let(:option) { List::CommandParser.new("connector:issue", ["config"], ['database', 'table'], nil, [File.join("spec", "td", "fixture", "bulk_load.yml"), '--database', database, '--table', table], true) } + let(:config) { {} } let(:expect_config) { {'out' => {'database' => database, 'table' => table}} } it 'show warning' do + subject + expect(stderr_io.string).to include '--database is obsolete option' expect(stderr_io.string).to include '--table is obsolete option' end @@ -113,11 +141,14 @@ module TreasureData::Command let(:option) { List::CommandParser.new("connector:issue", ["config", 'database', 'table'], [], nil, [database, table, File.join("spec", "td", "fixture", "bulk_load.yml")], true) } + let(:config) { {} } let(:expect_config) { {'out' => {'database' => database, 'table' => table}} } it 'no warning' do + subject + expect(stderr_io.string).not_to include '--database is obsolete option' expect(stderr_io.string).not_to include '--table is obsolete option' end From e5f207daaaa39ba82f9950d47480f76c444137fc Mon Sep 17 00:00:00 2001 From: takkanm Date: Thu, 20 Aug 2015 16:31:05 +0900 Subject: [PATCH 05/10] extract method and add warning --- lib/td/command/connector.rb | 24 ++++++++++++++++-------- spec/td/command/connector_spec.rb | 6 +++++- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/lib/td/command/connector.rb b/lib/td/command/connector.rb index f7414829..003eb380 100644 --- a/lib/td/command/connector.rb +++ b/lib/td/command/connector.rb @@ -113,14 +113,9 @@ def connector_issue(op) wait = exclude = false auto_create = false - op.on('--database DB_NAME', "(obsoleted)") { |s| - $stderr.puts '--database is obsolete option' - option_database = s - } - op.on('--table TABLE_NAME', "(obsoleted)") { |s| - $stderr.puts '--table is obsolete option' - option_table = s - } + on_with_obsolute_and_overwrite_config_warning(op, '--database DB_NAME') { |s| option_database = s } + on_with_obsolute_and_overwrite_config_warning(op, '--table TABLE_NAME') { |s| option_table = s } + op.on('--time-column COLUMN_NAME', "data partitioning key") { |s| time_column = s } # unnecessary but for backward compatibility op.on('-w', '--wait', 'wait for finishing the job', TrueClass) { |b| wait = b } op.on('-x', '--exclude', 'do not automatically retrieve the job result', TrueClass) { |b| exclude = b } @@ -161,6 +156,19 @@ def connector_issue(op) end end + def on_with_obsolute_and_overwrite_config_warning(op, *args, &block) + options = args.each_with_object([]) do |arg, o| + arg.split("\s").each do |word| + o << word if word =~ /\A-/ + end + end + + op.on(*args, '(obsoleted)') do |s| + $stderr.puts "#{options.join(',')} #{options.size > 1 ? 'are' : 'is'} obsolete option. Even if you wrote in the configuration file, #{s} is used." + block.call(s) + end + end + def overwrite_out_config(config, out_args) # TODO will not work once embulk implements multi-job config['out'] ||= {} diff --git a/spec/td/command/connector_spec.rb b/spec/td/command/connector_spec.rb index c1169f1c..93cf0752 100644 --- a/spec/td/command/connector_spec.rb +++ b/spec/td/command/connector_spec.rb @@ -116,7 +116,11 @@ module TreasureData::Command {'out' => {'database' => 'config_database', 'table' => 'config_table'}} } - it { expect { subject }.not_to raise_error } + it { + expect { subject }.not_to raise_error + expect(stderr_io.string).to include "#{database} is used." + expect(stderr_io.string).to include "#{table} is used." + } end end From 2474f19b0703d7026f85d407bdca4e5a98fc4cf8 Mon Sep 17 00:00:00 2001 From: takkanm Date: Thu, 20 Aug 2015 16:42:57 +0900 Subject: [PATCH 06/10] time-column option is obsolute --- lib/td/command/connector.rb | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/lib/td/command/connector.rb b/lib/td/command/connector.rb index 003eb380..f245631d 100644 --- a/lib/td/command/connector.rb +++ b/lib/td/command/connector.rb @@ -108,34 +108,30 @@ def connector_preview(op) end def connector_issue(op) - option_database = option_table = nil - time_column = nil - wait = exclude = false - auto_create = false + wait = exclude = false + auto_create = false + config_option = {} - on_with_obsolute_and_overwrite_config_warning(op, '--database DB_NAME') { |s| option_database = s } - on_with_obsolute_and_overwrite_config_warning(op, '--table TABLE_NAME') { |s| option_table = s } + on_with_obsolute_and_overwrite_config_warning(op, '--database DB_NAME') { |s| config_option['database'] = s } + on_with_obsolute_and_overwrite_config_warning(op, '--table TABLE_NAME') { |s| config_option['table'] = s } + on_with_obsolute_and_overwrite_config_warning(op, '--time-column COLUMN_NAME') { |s| config_option['time_column'] = s } - op.on('--time-column COLUMN_NAME', "data partitioning key") { |s| time_column = s } # unnecessary but for backward compatibility op.on('-w', '--wait', 'wait for finishing the job', TrueClass) { |b| wait = b } op.on('-x', '--exclude', 'do not automatically retrieve the job result', TrueClass) { |b| exclude = b } op.on('--auto-create-table', "Create table and database if doesn't exist", TrueClass) { |b| auto_create = b } - database, table, config_file = nil args = op.cmd_parse if args.instance_of? String config_file = args - database = option_database - table = option_table else - database, table, config_file = args + config_option['database'], config_option['table'], config_file = args end config = prepare_bulkload_job_config(config_file) - overwrite_out_config(config, 'database' => database, 'table' => table, 'time_column' => time_column) + overwrite_out_config(config, config_option) client = get_client() From d0791ba28e0f172a7e90319ad14bac3369661bdc Mon Sep 17 00:00:00 2001 From: takkanm Date: Tue, 25 Aug 2015 11:07:07 +0900 Subject: [PATCH 07/10] add comment --- lib/td/command/connector.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/td/command/connector.rb b/lib/td/command/connector.rb index f245631d..f5b8f193 100644 --- a/lib/td/command/connector.rb +++ b/lib/td/command/connector.rb @@ -138,6 +138,7 @@ def connector_issue(op) required('database', config['out']['database']) required('table', config['out']['table']) + # TODO need fix if embulk support multi-job if auto_create create_database_and_table_if_not_exist(client, config['out']['database'], config['out']['table']) end From d2e3ad4fff4b9dcbe9dc034cbe0902d3e34685a3 Mon Sep 17 00:00:00 2001 From: takkanm Date: Tue, 25 Aug 2015 11:13:42 +0900 Subject: [PATCH 08/10] unsupport command arguments for database and table Because, we recommend to write to config file these options. --- lib/td/command/connector.rb | 8 +------- spec/td/command/connector_spec.rb | 17 ----------------- 2 files changed, 1 insertion(+), 24 deletions(-) diff --git a/lib/td/command/connector.rb b/lib/td/command/connector.rb index f5b8f193..cb01c171 100644 --- a/lib/td/command/connector.rb +++ b/lib/td/command/connector.rb @@ -122,13 +122,7 @@ def connector_issue(op) auto_create = b } - args = op.cmd_parse - - if args.instance_of? String - config_file = args - else - config_option['database'], config_option['table'], config_file = args - end + config_file = op.cmd_parse config = prepare_bulkload_job_config(config_file) overwrite_out_config(config, config_option) diff --git a/spec/td/command/connector_spec.rb b/spec/td/command/connector_spec.rb index 93cf0752..210e776a 100644 --- a/spec/td/command/connector_spec.rb +++ b/spec/td/command/connector_spec.rb @@ -140,23 +140,6 @@ module TreasureData::Command expect(stderr_io.string).to include '--table is obsolete option' end end - - context 'set arguments' do - let(:option) { - List::CommandParser.new("connector:issue", ["config", 'database', 'table'], [], nil, [database, table, File.join("spec", "td", "fixture", "bulk_load.yml")], true) - } - let(:config) { {} } - let(:expect_config) { - {'out' => {'database' => database, 'table' => table}} - } - - it 'no warning' do - subject - - expect(stderr_io.string).not_to include '--database is obsolete option' - expect(stderr_io.string).not_to include '--table is obsolete option' - end - end end describe 'queueing job' do From 6d85cad62e9d6f4abfef75a2e66e6b4dfc3d0ddd Mon Sep 17 00:00:00 2001 From: takkanm Date: Tue, 25 Aug 2015 11:17:22 +0900 Subject: [PATCH 09/10] more infomation for warning. --- lib/td/command/connector.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/td/command/connector.rb b/lib/td/command/connector.rb index cb01c171..c2494ed5 100644 --- a/lib/td/command/connector.rb +++ b/lib/td/command/connector.rb @@ -155,7 +155,7 @@ def on_with_obsolute_and_overwrite_config_warning(op, *args, &block) end op.on(*args, '(obsoleted)') do |s| - $stderr.puts "#{options.join(',')} #{options.size > 1 ? 'are' : 'is'} obsolete option. Even if you wrote in the configuration file, #{s} is used." + $stderr.puts "#{options.join(',')} #{options.size > 1 ? 'are' : 'is'} obsolete option. You should write to configuration file. Even if you wrote in the configuration file, #{s} is used." block.call(s) end end From 3d68947e6487905fb636012e87619602d018e141 Mon Sep 17 00:00:00 2001 From: takkanm Date: Tue, 25 Aug 2015 16:45:03 +0900 Subject: [PATCH 10/10] fix preview message --- lib/td/command/connector.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/td/command/connector.rb b/lib/td/command/connector.rb index c2494ed5..8bcc1ef3 100644 --- a/lib/td/command/connector.rb +++ b/lib/td/command/connector.rb @@ -104,7 +104,7 @@ def connector_preview(op) $stdout.puts cmd_render_table(rows, :fields => fields, :render_format => op.render_format, :resize => false) $stdout.puts "Update #{config_file} and use '#{$prog} " + Config.cl_options_string + "connector:preview #{config_file}' to preview again." - $stdout.puts "Use '#{$prog} " + Config.cl_options_string + "connector:issue database_name table_name #{config_file}' to run Server-side bulk load." + $stdout.puts "Use '#{$prog} " + Config.cl_options_string + "connector:issue #{config_file}' to run data connector." end def connector_issue(op)