Repository: kudu Updated Branches: refs/heads/master 117e7d906 -> 8386ef9e2
Follow-up improvements to locate_row - Added test coverage for a string literal with a space in it. - Abbreviated the tests by adding a couple of lambdas. - Print out all tablet ids when multiple tablets match the primary key. This should never happen, but if it does this output will be useful. - Couple of other small, non-functional changes. Change-Id: I9f4cca87963eee1d6da45fcbd3c27f782c35e6f6 Reviewed-on: http://gerrit.cloudera.org:8080/11715 Reviewed-by: Adar Dembo <a...@cloudera.com> Tested-by: Kudu Jenkins Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/f0ed88bb Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/f0ed88bb Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/f0ed88bb Branch: refs/heads/master Commit: f0ed88bbe728d7a739a0a0ecce35aaf7535f4933 Parents: 117e7d9 Author: Will Berkeley <wdberke...@gmail.org> Authored: Wed Oct 17 14:27:48 2018 -0700 Committer: Will Berkeley <wdberke...@gmail.com> Committed: Thu Oct 18 00:18:25 2018 +0000 ---------------------------------------------------------------------- src/kudu/tools/kudu-admin-test.cc | 194 +++++++++---------------------- src/kudu/tools/tool_action_table.cc | 20 +++- 2 files changed, 71 insertions(+), 143 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/f0ed88bb/src/kudu/tools/kudu-admin-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tools/kudu-admin-test.cc b/src/kudu/tools/kudu-admin-test.cc index d68fb4b..ecf6323 100644 --- a/src/kudu/tools/kudu-admin-test.cc +++ b/src/kudu/tools/kudu-admin-test.cc @@ -1502,7 +1502,7 @@ TEST_F(AdminCliTest, TestLocateRow) { }, &stdout, &stderr); ASSERT_TRUE(s.ok()) << ToolRunInfo(s, stdout, stderr); - // Grab list of tablet_ids from any tserver and check the output. + // Grab list of tablet_ids from the tserver and check the output. vector<TServerDetails*> tservers; vector<string> tablet_ids; AppendValuesFromMap(tablet_servers_, &tservers); @@ -1512,45 +1512,30 @@ TEST_F(AdminCliTest, TestLocateRow) { ASSERT_EQ(1, tablet_ids.size()); ASSERT_STR_CONTAINS(stdout, tablet_ids[0]); - // Test a couple of error cases. + // Test a few error cases. + const auto check_bad_input = [&](const string& json, const string& error) { + string out, err; + Status s = RunKuduTool({ + "table", + "locate_row", + cluster_->master()->bound_rpc_addr().ToString(), + kTableId, + json, + }, &out, &err); + ASSERT_TRUE(s.IsRuntimeError()); + ASSERT_STR_CONTAINS(err, error); + }; + // String instead of int. - stdout.clear(); - stderr.clear(); - s = RunKuduTool({ - "table", - "locate_row", - cluster_->master()->bound_rpc_addr().ToString(), - kTableId, - "[\"foo\"]" - }, &stdout, &stderr); - ASSERT_TRUE(s.IsRuntimeError()); - ASSERT_STR_CONTAINS(stderr, "unable to parse"); + NO_FATALS(check_bad_input("[\"foo\"]", "unable to parse")); // Float instead of int. - stdout.clear(); - stderr.clear(); - s = RunKuduTool({ - "table", - "locate_row", - cluster_->master()->bound_rpc_addr().ToString(), - kTableId, - "[1.2]" - }, &stdout, &stderr); - ASSERT_TRUE(s.IsRuntimeError()); - ASSERT_STR_CONTAINS(stderr, "unable to parse"); + NO_FATALS(check_bad_input("[1.2]", "unable to parse")); // Overflow (recall the key is INT32). - stdout.clear(); - stderr.clear(); - s = RunKuduTool({ - "table", - "locate_row", - cluster_->master()->bound_rpc_addr().ToString(), - kTableId, - Substitute("[$0]", std::to_string(std::numeric_limits<int64_t>::max())) - }, &stdout, &stderr); - ASSERT_TRUE(s.IsRuntimeError()); - ASSERT_STR_CONTAINS(stderr, "out of range"); + NO_FATALS(check_bad_input( + Substitute("[$0]", std::to_string(std::numeric_limits<int64_t>::max())), + "out of range")); } TEST_F(AdminCliTest, TestLocateRowMore) { @@ -1608,7 +1593,7 @@ TEST_F(AdminCliTest, TestLocateRowMore) { "locate_row", cluster_->master()->bound_rpc_addr().ToString(), kAnotherTableId, - "[\"foo\",0]" + "[\"foo bar\",0]" }, &stdout, &stderr); ASSERT_TRUE(s.ok()) << ToolRunInfo(s, stdout, stderr); StripWhiteSpace(&stdout); @@ -1633,117 +1618,50 @@ TEST_F(AdminCliTest, TestLocateRowMore) { << "expected to find tablet id " << tablet_id_for_2; ASSERT_NE(tablet_id_for_0, tablet_id_for_2); + // Test a few error cases. + const auto check_bad_input = [&](const string& json, const string& error) { + string out, err; + Status s = RunKuduTool({ + "table", + "locate_row", + cluster_->master()->bound_rpc_addr().ToString(), + kAnotherTableId, + json, + }, &out, &err); + ASSERT_TRUE(s.IsRuntimeError()); + ASSERT_STR_CONTAINS(err, error); + }; + // Test locating a row lying in a non-covered range. - stdout.clear(); - stderr.clear(); - s = RunKuduTool({ - "table", - "locate_row", - cluster_->master()->bound_rpc_addr().ToString(), - kAnotherTableId, - "[\"foo\",1]" - }, &stdout, &stderr); - ASSERT_TRUE(s.IsRuntimeError()) << ToolRunInfo(s, stdout, stderr); - ASSERT_STR_CONTAINS(stderr, "row does not belong to any currently existing tablet"); + NO_FATALS(check_bad_input( + "[\"foo\",1]", + "row does not belong to any currently existing tablet")); // Test providing a missing or incomplete primary key. - stdout.clear(); - stderr.clear(); - s = RunKuduTool({ - "table", - "locate_row", - cluster_->master()->bound_rpc_addr().ToString(), - kAnotherTableId, - "[]" - }, &stdout, &stderr); - ASSERT_TRUE(s.IsRuntimeError()) << ToolRunInfo(s, stdout, stderr); - ASSERT_STR_CONTAINS( - stderr, - "wrong number of key columns specified: expected 2 but received 0"); - - stdout.clear(); - stderr.clear(); - s = RunKuduTool({ - "table", - "locate_row", - cluster_->master()->bound_rpc_addr().ToString(), - kAnotherTableId, - "[\"foo\"]" - }, &stdout, &stderr); - ASSERT_TRUE(s.IsRuntimeError()) << ToolRunInfo(s, stdout, stderr); - ASSERT_STR_CONTAINS( - stderr, - "wrong number of key columns specified: expected 2 but received 1"); - - stdout.clear(); - stderr.clear(); - s = RunKuduTool({ - "table", - "locate_row", - cluster_->master()->bound_rpc_addr().ToString(), - kAnotherTableId, - "[\"foo\",]" - }, &stdout, &stderr); - ASSERT_TRUE(s.IsRuntimeError()) << ToolRunInfo(s, stdout, stderr); - ASSERT_STR_CONTAINS( - stderr, - "JSON text is corrupt"); + NO_FATALS(check_bad_input( + "[]", + "wrong number of key columns specified: expected 2 but received 0")); + NO_FATALS(check_bad_input( + "[\"foo\"]", + "wrong number of key columns specified: expected 2 but received 1")); // Test providing too many key column values. - stdout.clear(); - stderr.clear(); - s = RunKuduTool({ - "table", - "locate_row", - cluster_->master()->bound_rpc_addr().ToString(), - kAnotherTableId, - "[\"foo\",2,\"bar\"]" - }, &stdout, &stderr); - ASSERT_TRUE(s.IsRuntimeError()) << ToolRunInfo(s, stdout, stderr); - ASSERT_STR_CONTAINS( - stderr, - "wrong number of key columns specified: expected 2 but received 3"); + NO_FATALS(check_bad_input( + "[\"foo\",2,\"bar\"]", + "wrong number of key columns specified: expected 2 but received 3")); // Test providing an invalid value for a key column when there's multiple // key columns. - stdout.clear(); - stderr.clear(); - s = RunKuduTool({ - "table", - "locate_row", - cluster_->master()->bound_rpc_addr().ToString(), - kAnotherTableId, - "[\"foo\",\"bar\"]" - }, &stdout, &stderr); - ASSERT_TRUE(s.IsRuntimeError()) << ToolRunInfo(s, stdout, stderr); - ASSERT_STR_CONTAINS(stderr, "unable to parse"); + NO_FATALS(check_bad_input("[\"foo\",\"bar\"]", "unable to parse")); // Test providing bad json. - stdout.clear(); - stderr.clear(); - s = RunKuduTool({ - "table", - "locate_row", - cluster_->master()->bound_rpc_addr().ToString(), - kAnotherTableId, - "[" - }, &stdout, &stderr); - ASSERT_TRUE(s.IsRuntimeError()) << ToolRunInfo(s, stdout, stderr); - ASSERT_STR_CONTAINS(stderr, "JSON text is corrupt"); + NO_FATALS(check_bad_input("[", "JSON text is corrupt")); + NO_FATALS(check_bad_input("[\"foo\",]", "JSON text is corrupt")); // Test providing valid JSON that's not an array. - stdout.clear(); - stderr.clear(); - s = RunKuduTool({ - "table", - "locate_row", - cluster_->master()->bound_rpc_addr().ToString(), - kAnotherTableId, - "{ \"key_hash\" : \"foo\", \"key_range\" : 2 }" - }, &stdout, &stderr); - ASSERT_TRUE(s.IsRuntimeError()) << ToolRunInfo(s, stdout, stderr); - ASSERT_STR_CONTAINS(stderr, - "Wrong type during field extraction: expected object array"); + NO_FATALS(check_bad_input( + "{ \"key_hash\" : \"foo\", \"key_range\" : 2 }", + "Wrong type during field extraction: expected object array")); } TEST_F(AdminCliTest, TestLocateRowAndCheckRowPresence) { @@ -1794,8 +1712,10 @@ TEST_F(AdminCliTest, TestLocateRowAndCheckRowPresence) { // Test the case when the row exists. Since the scan is done by a subprocess // using a different client instance, it's possible the scan will not - // immediately retrieve the row even though the write has already succeeded, - // so we ASSERT_EVENTUALLY. + // immediately retrieve the row even though the write has already succeeded. + // This use case is what timestamp propagation is for, but there's no way to + // propagate a timestamp to a tool (and there shouldn't be). Instead, we + // ASSERT_EVENTUALLY. ASSERT_EVENTUALLY([&]() { stdout.clear(); stderr.clear(); http://git-wip-us.apache.org/repos/asf/kudu/blob/f0ed88bb/src/kudu/tools/tool_action_table.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tools/tool_action_table.cc b/src/kudu/tools/tool_action_table.cc index 2156dba..310cb97 100644 --- a/src/kudu/tools/tool_action_table.cc +++ b/src/kudu/tools/tool_action_table.cc @@ -71,6 +71,7 @@ using client::KuduScanTokenBuilder; using client::KuduTable; using client::KuduTableAlterer; using client::internal::ReplicaController; +using std::cerr; using std::cout; using std::endl; using std::string; @@ -204,7 +205,9 @@ Status LocateRow(const RunnerContext& context) { JsonReader reader(row_str); RETURN_NOT_OK(reader.Init()); vector<const rapidjson::Value*> values; - RETURN_NOT_OK(reader.ExtractObjectArray(reader.root(), nullptr, &values)); + RETURN_NOT_OK(reader.ExtractObjectArray(reader.root(), + /*field=*/nullptr, + &values)); const auto& schema = table->schema(); vector<int> key_indexes; @@ -230,7 +233,7 @@ Status LocateRow(const RunnerContext& context) { case KuduColumnSchema::UNIXTIME_MICROS: { int64_t value; RETURN_NOT_OK_PREPEND( - reader.ExtractInt64(values[i], nullptr, &value), + reader.ExtractInt64(values[i], /*field=*/nullptr, &value), Substitute("unable to parse value for column '$0' of type $1", col_name, KuduColumnSchema::DataTypeToString(type))); @@ -244,7 +247,7 @@ Status LocateRow(const RunnerContext& context) { case KuduColumnSchema::STRING: { string value; RETURN_NOT_OK_PREPEND( - reader.ExtractString(values[i], nullptr, &value), + reader.ExtractString(values[i], /*field=*/nullptr, &value), Substitute("unable to parse value for column '$0' of type $1", col_name, KuduColumnSchema::DataTypeToString(type))); @@ -286,9 +289,14 @@ Status LocateRow(const RunnerContext& context) { return Status::NotFound("row does not belong to any currently existing tablet"); } if (tokens.size() > 1) { - // This should be impossible. - return Status::IllegalState( - "all primary key columns specified but more than one matching tablet?"); + // This should be impossible. But if it does happen, we'd like to know what + // all the matching tablets were. + for (const auto& token : tokens) { + cerr << token->tablet().id() << endl; + } + return Status::IllegalState(Substitute( + "all primary key columns specified but found $0 matching tablets!", + tokens.size())); } cout << tokens[0]->tablet().id() << endl;
