kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mpe...@apache.org
Subject [1/3] kudu git commit: Follow-up improvements to locate_row
Date Thu, 18 Oct 2018 02:09:11 GMT
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 <adar@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 <wdberkeley@gmail.org>
Authored: Wed Oct 17 14:27:48 2018 -0700
Committer: Will Berkeley <wdberkeley@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;
 


Mime
View raw message