You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by mp...@apache.org on 2019/03/26 22:59:14 UTC

[kudu] branch master updated (cf8cb39 -> e8be768)

This is an automated email from the ASF dual-hosted git repository.

mpercy pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git.


    from cf8cb39  java: normalize test log4j.properties with INFO logging
     new d533dcb  dist-test: improve error message when no tests run
     new c355e8e  tablet_service: improve error handling specificity
     new e8be768  KUDU-2744: Expose RPC interface for diff scan

The 3 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.


Summary of changes:
 build-support/dist_test.py                         |   4 +
 .../integration-tests/tablet_history_gc-itest.cc   |   3 +-
 src/kudu/tools/ksck_remote-test.cc                 |   2 +-
 src/kudu/tserver/tablet_server-test-base.cc        |  27 +--
 src/kudu/tserver/tablet_server-test-base.h         |   9 +-
 src/kudu/tserver/tablet_server-test.cc             | 205 ++++++++++++++++++++-
 src/kudu/tserver/tablet_service.cc                 | 169 +++++++++++------
 src/kudu/tserver/tablet_service.h                  |  11 +-
 src/kudu/tserver/tserver.proto                     |  36 +++-
 9 files changed, 389 insertions(+), 77 deletions(-)


[kudu] 03/03: KUDU-2744: Expose RPC interface for diff scan

Posted by mp...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

mpercy pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git

commit e8be768c1b7f3a1cc67596e0b554f947c41f47bb
Author: Mike Percy <mp...@apache.org>
AuthorDate: Mon Feb 11 12:07:21 2019 -0800

    KUDU-2744: Expose RPC interface for diff scan
    
    This patch exposes an RPC interface for diff scans by adding a new field
    "snap_start_timestamp" to NewScanRequestPB. Additional validation is
    performed to ensure this timestamp is not prior to the ancient history
    mark.
    
    Includes a randomized test of the raw RPC interface.
    
    Change-Id: I9602b549dc499f92b2069b297e66f366e51fb312
    Reviewed-on: http://gerrit.cloudera.org:8080/12592
    Tested-by: Mike Percy <mp...@apache.org>
    Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
 .../integration-tests/tablet_history_gc-itest.cc   |   3 +-
 src/kudu/tools/ksck_remote-test.cc                 |   4 +-
 src/kudu/tserver/tablet_server-test.cc             | 192 ++++++++++++++++++++-
 src/kudu/tserver/tablet_service.cc                 |  94 ++++++++--
 src/kudu/tserver/tablet_service.h                  |   8 +
 src/kudu/tserver/tserver.proto                     |  31 +++-
 6 files changed, 309 insertions(+), 23 deletions(-)

diff --git a/src/kudu/integration-tests/tablet_history_gc-itest.cc b/src/kudu/integration-tests/tablet_history_gc-itest.cc
index 0592337..c3c97fb 100644
--- a/src/kudu/integration-tests/tablet_history_gc-itest.cc
+++ b/src/kudu/integration-tests/tablet_history_gc-itest.cc
@@ -133,7 +133,8 @@ TEST_F(TabletHistoryGcITest, TestSnapshotScanBeforeAHM) {
   ASSERT_OK(scanner.SetReadMode(KuduScanner::READ_AT_SNAPSHOT));
   Status s = scanner.Open();
   ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
-  ASSERT_STR_CONTAINS(s.ToString(), "Snapshot timestamp is earlier than the ancient history mark");
+  ASSERT_STR_CONTAINS(s.ToString(),
+                      "snapshot scan end timestamp is earlier than the ancient history mark");
 }
 
 // Check that the maintenance manager op to delete undo deltas actually deletes them.
diff --git a/src/kudu/tools/ksck_remote-test.cc b/src/kudu/tools/ksck_remote-test.cc
index 55c1ca7..ffce813 100644
--- a/src/kudu/tools/ksck_remote-test.cc
+++ b/src/kudu/tools/ksck_remote-test.cc
@@ -475,8 +475,8 @@ TEST_F(RemoteKsckTest, TestChecksumSnapshotLastingLongerThanAHM) {
   ASSERT_TRUE(s.IsAborted()) << s.ToString();
   ASSERT_OK(ksck_->PrintResults());
   ASSERT_STR_CONTAINS(err_stream_.str(),
-                      "Invalid argument: cannot verify timestamp: "
-                      "Snapshot timestamp is earlier than the ancient history mark.");
+                      "Invalid argument: snapshot scan end timestamp is earlier than "
+                      "the ancient history mark.");
 
   // Now let's try again using the special current timestamp, which will run
   // checksums using timestamps updated from the servers, and should succeed.
diff --git a/src/kudu/tserver/tablet_server-test.cc b/src/kudu/tserver/tablet_server-test.cc
index a4604a3..e055dc5 100644
--- a/src/kudu/tserver/tablet_server-test.cc
+++ b/src/kudu/tserver/tablet_server-test.cc
@@ -22,6 +22,7 @@
 
 #include <algorithm>
 #include <cstdint>
+#include <map>
 #include <memory>
 #include <set>
 #include <sstream>
@@ -61,6 +62,7 @@
 #include "kudu/gutil/callback.h"
 #include "kudu/gutil/casts.h"
 #include "kudu/gutil/gscoped_ptr.h"
+#include "kudu/gutil/map-util.h"
 #include "kudu/gutil/port.h"
 #include "kudu/gutil/ref_counted.h"
 #include "kudu/gutil/stringprintf.h"
@@ -74,6 +76,7 @@
 #include "kudu/server/rpc_server.h"
 #include "kudu/server/server_base.pb.h"
 #include "kudu/server/server_base.proxy.h"
+#include "kudu/tablet/local_tablet_writer.h"
 #include "kudu/tablet/metadata.pb.h"
 #include "kudu/tablet/tablet.h"
 #include "kudu/tablet/tablet_metadata.h"
@@ -121,10 +124,13 @@ using kudu::pb_util::SecureShortDebugString;
 using kudu::rpc::Messenger;
 using kudu::rpc::MessengerBuilder;
 using kudu::rpc::RpcController;
+using kudu::tablet::LocalTabletWriter;
 using kudu::tablet::RowSetDataPB;
 using kudu::tablet::Tablet;
 using kudu::tablet::TabletReplica;
 using kudu::tablet::TabletSuperBlockPB;
+using std::map;
+using std::pair;
 using std::set;
 using std::shared_ptr;
 using std::string;
@@ -186,7 +192,7 @@ class TabletServerTest : public TabletServerTestBase {
   // Starts the tablet server, override to start it later.
   virtual void SetUp() OVERRIDE {
     NO_FATALS(TabletServerTestBase::SetUp());
-    NO_FATALS(StartTabletServer(/* num_data_dirs */ 1));
+    NO_FATALS(StartTabletServer(/*num_data_dirs=*/1));
   }
 
   void DoOrderedScanTest(const Schema& projection, const string& expected_rows_as_string);
@@ -2379,6 +2385,190 @@ TEST_F(TabletServerTest, TestScanWithEncodedPredicates) {
             results.back());
 }
 
+// Test for diff scan RPC interface.
+TEST_F(TabletServerTest, TestDiffScan) {
+  // Insert 100 rows with the usual pattern.
+  const int kStartRow = 0;
+  const int kNumRows = 1000;
+  const int kNumToUpdate = 200;
+  const int kNumToDelete = 100;
+  InsertTestRowsDirect(kStartRow, kNumRows);
+  Timestamp before_mutations = tablet_replica_->clock()->Now();
+
+  // Structure: key -> {val, is_deleted}
+  map<int32_t, pair<int32_t, bool>> expected;
+
+  vector<int32_t> keys;
+  keys.reserve(kNumRows);
+  for (int32_t i = 0; i < kNumRows; i++) {
+    keys.emplace_back(i);
+  }
+
+  // Update some random rows.
+  LocalTabletWriter writer(tablet_replica_->tablet(), &schema_);
+  std::random_shuffle(keys.begin(), keys.end());
+  for (int i = 0; i < kNumToUpdate; i++) {
+    KuduPartialRow row(&schema_);
+    int32_t key = keys[i];
+    CHECK_OK(row.SetInt32(0, key));
+    int32_t new_val = key * 3;
+    CHECK_OK(row.SetInt32(1, new_val));
+    InsertOrDie(&expected, key, pair<int32_t, bool>(new_val, false));
+    CHECK_OK(writer.Update(row));
+  }
+
+  // Delete some random rows.
+  std::random_shuffle(keys.begin(), keys.end());
+  for (int i = 0; i < kNumToDelete; i++) {
+    KuduPartialRow row(&schema_);
+    int32_t key = keys[i];
+    CHECK_OK(row.SetInt32(0, key));
+    EmplaceOrUpdate(&expected, key, pair<int32_t, bool>(0 /* ignored */, true));
+    CHECK_OK(writer.Delete(row));
+  }
+
+  Timestamp after_mutations = tablet_replica_->clock()->Now();
+
+  ScanRequestPB req;
+  ScanResponsePB resp;
+  RpcController rpc;
+
+  // Build a projection with an IS_DELETED column.
+  SchemaBuilder builder(*tablet_replica_->tablet()->schema());
+  const bool kIsDeletedDefault = false;
+  ASSERT_OK(builder.AddColumn("is_deleted", IS_DELETED,
+                              /*is_nullable=*/ false,
+                              /*read_default=*/ &kIsDeletedDefault,
+                              /*write_default=*/ nullptr));
+  Schema projection = builder.BuildWithoutIds();
+
+  // Start scan.
+  auto* new_scan = req.mutable_new_scan_request();
+  new_scan->set_tablet_id(kTabletId);
+  ASSERT_OK(SchemaToColumnPBs(projection, new_scan->mutable_projected_columns()));
+  new_scan->set_read_mode(READ_AT_SNAPSHOT);
+  new_scan->set_order_mode(ORDERED);
+  new_scan->set_snap_start_timestamp(before_mutations.ToUint64());
+  new_scan->set_snap_timestamp(after_mutations.ToUint64());
+
+  int call_seq_id = 0;
+  {
+    req.set_call_seq_id(call_seq_id);
+    req.set_batch_size_bytes(0); // So it won't return data right away.
+    SCOPED_TRACE(SecureDebugString(req));
+    ASSERT_OK(proxy_->Scan(req, &resp, &rpc));
+    SCOPED_TRACE(SecureDebugString(resp));
+    ASSERT_FALSE(resp.has_error());
+    ASSERT_EQ(0, resp.data().num_rows());
+  }
+
+  // Consume the scan results and validate that the values are as expected.
+  req.clear_new_scan_request();
+  req.set_scanner_id(resp.scanner_id());
+
+  vector<string> results;
+  while (resp.has_more_results()) {
+    rpc.Reset();
+    req.set_call_seq_id(++call_seq_id);
+    SCOPED_TRACE(SecureDebugString(req));
+    ASSERT_OK(proxy_->Scan(req, &resp, &rpc));
+    SCOPED_TRACE(SecureDebugString(resp));
+    ASSERT_FALSE(resp.has_error());
+    NO_FATALS(StringifyRowsFromResponse(projection, rpc, &resp, &results));
+  }
+
+  // Verify that the scan results match what we expected.
+  ASSERT_EQ(expected.size(), results.size());
+  int i = 0;
+  for (const auto& entry : expected) {
+    int32_t key = entry.first;
+    int32_t val = entry.second.first;
+    bool is_deleted = entry.second.second;
+    string val_str = Substitute("$0", val);
+    if (is_deleted) {
+      val_str = ".*"; // Match any value on deleted values.
+    }
+    ASSERT_STR_MATCHES(results[i++],
+        Substitute("^\\(int32 key=$0, int32 int_val=$1, string string_val=\"hello $0\", "
+                   "is_deleted is_deleted=$2\\)$$", key, val_str, is_deleted));
+  }
+}
+
+// Send various "bad" diff scan requests and validate that we catch the errors
+// and respond with reasonable error messages.
+TEST_F(TabletServerTest, TestDiffScanErrors) {
+  Timestamp before_insert = tablet_replica_->clock()->Now();
+  InsertTestRowsDirect(/*start_row=*/0, /*num_rows=*/100);
+  Timestamp after_insert = tablet_replica_->clock()->Now();
+
+  // Build a projection with an IS_DELETED column.
+  SchemaBuilder builder(*tablet_replica_->tablet()->schema());
+  const bool kIsDeletedDefault = false;
+  ASSERT_OK(builder.AddColumn("is_deleted", IS_DELETED,
+                              /*is_nullable=*/ false,
+                              /*read_default=*/ &kIsDeletedDefault,
+                              /*write_default=*/ nullptr));
+  Schema projection = builder.BuildWithoutIds();
+
+  ScanRequestPB req;
+  ScanResponsePB resp;
+  RpcController rpc;
+
+  // Set up the RPC request.
+  auto* new_scan = req.mutable_new_scan_request();
+  new_scan->set_tablet_id(kTabletId);
+  ASSERT_OK(SchemaToColumnPBs(projection, new_scan->mutable_projected_columns()));
+  new_scan->set_snap_start_timestamp(before_insert.ToUint64());
+  new_scan->set_snap_timestamp(after_insert.ToUint64());
+
+  req.set_call_seq_id(0);
+  req.set_batch_size_bytes(0); // So it won't return data right away.
+
+  // Send a scan request to the server and assert
+  auto req_assert_invalid_argument = [&](const TabletServerErrorPB::Code expected_code,
+                                         const string& expected_msg) {
+    rpc.Reset();
+    ASSERT_OK(proxy_->Scan(req, &resp, &rpc));
+    ASSERT_TRUE(resp.has_error());
+    ASSERT_EQ(expected_code, resp.error().code())
+      << "Expected " << TabletServerErrorPB::Code_Name(expected_code)
+      << ", got " << TabletServerErrorPB::Code_Name(resp.error().code());
+    Status s = StatusFromPB(resp.error().status());
+    ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+    ASSERT_STR_CONTAINS(s.ToString(), expected_msg);
+  };
+
+  // Attempt to start a diff scan with an illegal scan mode.
+  for (ReadMode read_mode : {READ_YOUR_WRITES, READ_LATEST}) {
+    new_scan->set_read_mode(read_mode);
+    NO_FATALS(req_assert_invalid_argument(TabletServerErrorPB::INVALID_SCAN_SPEC,
+        "scan start timestamp is only supported in READ_AT_SNAPSHOT read mode"));
+  }
+  new_scan->set_read_mode(READ_AT_SNAPSHOT);
+
+  // Attempt to start a diff scan with an illegal order mode.
+  new_scan->set_order_mode(UNORDERED);
+  NO_FATALS(req_assert_invalid_argument(TabletServerErrorPB::INVALID_SCAN_SPEC,
+      "scan start timestamp is only supported in ORDERED order mode"));
+  new_scan->set_order_mode(ORDERED);
+
+  // Attempt to start a diff scan with a too-early start timestamp.
+  new_scan->set_snap_start_timestamp(0); // Way before the AHM.
+  NO_FATALS(req_assert_invalid_argument(TabletServerErrorPB::INVALID_SNAPSHOT,
+      "snapshot scan start timestamp is earlier than the ancient history mark"));
+
+  // Attempt to start a diff scan with a too-early end timestamp.
+  new_scan->set_snap_timestamp(1);
+  NO_FATALS(req_assert_invalid_argument(TabletServerErrorPB::INVALID_SNAPSHOT,
+      "snapshot scan end timestamp is earlier than the ancient history mark"));
+
+  // Attempt to start a diff scan with a start timestamp higher than the end
+  // timestamp.
+  new_scan->set_snap_start_timestamp(after_insert.ToUint64());
+  new_scan->set_snap_timestamp(before_insert.ToUint64());
+  NO_FATALS(req_assert_invalid_argument(TabletServerErrorPB::INVALID_SNAPSHOT,
+      "must be less than or equal to end timestamp"));
+}
 
 // Test requesting more rows from a scanner which doesn't exist
 TEST_F(TabletServerTest, TestBadScannerID) {
diff --git a/src/kudu/tserver/tablet_service.cc b/src/kudu/tserver/tablet_service.cc
index e710e37..deafd9a 100644
--- a/src/kudu/tserver/tablet_service.cc
+++ b/src/kudu/tserver/tablet_service.cc
@@ -192,6 +192,7 @@ using kudu::security::TokenVerifier;
 using kudu::security::TokenPB;
 using kudu::server::ServerBase;
 using kudu::tablet::AlterSchemaTransactionState;
+using kudu::tablet::MvccSnapshot;
 using kudu::tablet::TABLET_DATA_COPYING;
 using kudu::tablet::TABLET_DATA_DELETED;
 using kudu::tablet::TABLET_DATA_TOMBSTONED;
@@ -2158,15 +2159,17 @@ namespace {
 // Checks if 'timestamp' is before the tablet's AHM if this is a
 // READ_AT_SNAPSHOT/READ_YOUR_WRITES scan. Returns Status::OK() if it's
 // not or Status::InvalidArgument() if it is.
-Status VerifyNotAncientHistory(Tablet* tablet, ReadMode read_mode, Timestamp timestamp) {
+Status VerifyNotAncientHistory(Tablet* tablet, ReadMode read_mode, Timestamp timestamp,
+                               const string& timestamp_desc) {
   tablet::HistoryGcOpts history_gc_opts = tablet->GetHistoryGcOpts();
   if ((read_mode == READ_AT_SNAPSHOT || read_mode == READ_YOUR_WRITES) &&
       history_gc_opts.IsAncientHistory(timestamp)) {
     return Status::InvalidArgument(
-        Substitute("Snapshot timestamp is earlier than the ancient history mark. Consider "
-                       "increasing the value of the configuration parameter "
-                       "--tablet_history_max_age_sec. Snapshot timestamp: $0 "
-                       "Ancient History Mark: $1 Physical time difference: $2",
+        Substitute("$0 is earlier than the ancient history mark. Consider "
+                   "increasing the value of the configuration parameter "
+                   "--tablet_history_max_age_sec. Snapshot timestamp: $1 "
+                   "Ancient History Mark: $2 Physical time difference: $3",
+                   timestamp_desc,
                    tablet->clock()->Stringify(timestamp),
                    tablet->clock()->Stringify(history_gc_opts.ancient_history_mark()),
                    tablet->clock()->GetPhysicalComponentDifference(
@@ -2174,6 +2177,28 @@ Status VerifyNotAncientHistory(Tablet* tablet, ReadMode read_mode, Timestamp tim
   }
   return Status::OK();
 }
+
+// Verify that the start (if specified) and end snapshot timestamps are legal
+// to read by checking against the ancient history mark and ensuring that the
+// start timestamp is earlier than the end timestamp.
+Status VerifyLegalSnapshotTimestamps(Tablet* tablet, ReadMode read_mode,
+                                     const boost::optional<Timestamp>& snap_start_timestamp,
+                                     const Timestamp& snap_end_timestamp) {
+  RETURN_NOT_OK(VerifyNotAncientHistory(tablet, read_mode, snap_end_timestamp,
+                                        "snapshot scan end timestamp"));
+  if (snap_start_timestamp) {
+    // Validate diff scan start timestamp, if set.
+    RETURN_NOT_OK(VerifyNotAncientHistory(tablet, read_mode, *snap_start_timestamp,
+                                          "snapshot scan start timestamp"));
+    if (snap_start_timestamp->CompareTo(snap_end_timestamp) > 0) {
+      return Status::InvalidArgument(
+          Substitute("start timestamp ($0) must be less than or equal to end timestamp ($1)",
+                     tablet->clock()->Stringify(*snap_start_timestamp),
+                     tablet->clock()->Stringify(snap_end_timestamp)));
+    }
+  }
+  return Status::OK();
+}
 } // anonymous namespace
 
 // Start a new scan.
@@ -2289,6 +2314,9 @@ Status TabletServiceImpl::HandleNewScanRequest(TabletReplica* replica,
     *error_code = TabletServerErrorPB::TABLET_NOT_RUNNING;
     return s;
   }
+
+  boost::optional<Timestamp> snap_start_timestamp;
+
   {
     TRACE("Creating iterator");
     TRACE_EVENT0("tserver", "Create iterator");
@@ -2299,6 +2327,11 @@ Status TabletServiceImpl::HandleNewScanRequest(TabletReplica* replica,
         return Status::NotSupported("Unknown read mode.");
       }
       case READ_LATEST: {
+        if (scan_pb.has_snap_start_timestamp()) {
+          *error_code = TabletServerErrorPB::INVALID_SCAN_SPEC;
+          return Status::InvalidArgument("scan start timestamp is only supported "
+                                         "in READ_AT_SNAPSHOT read mode");
+        }
         s = tablet->NewRowIterator(projection, &iter);
         break;
       }
@@ -2306,7 +2339,7 @@ Status TabletServiceImpl::HandleNewScanRequest(TabletReplica* replica,
       case READ_AT_SNAPSHOT: {
         scoped_refptr<consensus::TimeManager> time_manager = replica->time_manager();
         s = HandleScanAtSnapshot(scan_pb, rpc_context, projection, tablet.get(),
-                                 time_manager.get(), &iter, snap_timestamp,
+                                 time_manager.get(), &iter, &snap_start_timestamp, snap_timestamp,
                                  error_code);
         break;
       }
@@ -2368,8 +2401,10 @@ Status TabletServiceImpl::HandleNewScanRequest(TabletReplica* replica,
   // Now that we have initialized our row iterator at a snapshot, return an
   // error if the snapshot timestamp was prior to the ancient history mark.
   // We have to check after we open the iterator in order to avoid a TOCTOU
-  // error.
-  RETURN_NOT_OK_EVAL(VerifyNotAncientHistory(tablet.get(), scan_pb.read_mode(), *snap_timestamp),
+  // error since it's possible that initializing the row iterator could race
+  // against the tablet history GC maintenance task.
+  RETURN_NOT_OK_EVAL(VerifyLegalSnapshotTimestamps(tablet.get(), scan_pb.read_mode(),
+                                                   snap_start_timestamp, *snap_timestamp),
                      *error_code = TabletServerErrorPB::INVALID_SNAPSHOT);
 
   *has_more_results = iter->HasNext() && !scanner->has_fulfilled_limit();
@@ -2600,6 +2635,7 @@ Status TabletServiceImpl::HandleScanAtSnapshot(const NewScanRequestPB& scan_pb,
                                                Tablet* tablet,
                                                consensus::TimeManager* time_manager,
                                                unique_ptr<RowwiseIterator>* iter,
+                                               boost::optional<Timestamp>* snap_start_timestamp,
                                                Timestamp* snap_timestamp,
                                                TabletServerErrorPB::Code* error_code) {
   switch (scan_pb.read_mode()) {
@@ -2664,10 +2700,43 @@ Status TabletServiceImpl::HandleScanAtSnapshot(const NewScanRequestPB& scan_pb,
   opts.projection = &projection;
   opts.snap_to_include = snap;
   opts.order = scan_pb.order_mode();
+
+  boost::optional<Timestamp> tmp_snap_start_timestamp;
+  if (scan_pb.has_snap_start_timestamp()) {
+    if (scan_pb.read_mode() != READ_AT_SNAPSHOT) {
+      // TODO(mpercy): Should we allow READ_YOUR_WRITES mode? There is no
+      // obvious use for it, but also no obvious reason not to support it,
+      // except for the fact that we would also have to test it.
+      *error_code = TabletServerErrorPB::INVALID_SCAN_SPEC;
+      return Status::InvalidArgument("scan start timestamp is only supported "
+                                     "in READ_AT_SNAPSHOT read mode");
+    }
+    if (scan_pb.order_mode() != ORDERED) {
+      *error_code = TabletServerErrorPB::INVALID_SCAN_SPEC;
+      return Status::InvalidArgument("scan start timestamp is only supported "
+                                     "in ORDERED order mode");
+    }
+    tmp_snap_start_timestamp = Timestamp(scan_pb.snap_start_timestamp());
+    opts.snap_to_exclude = MvccSnapshot(*tmp_snap_start_timestamp);
+    opts.include_deleted_rows = true;
+  }
+
+  // Before we open / wait on anything check that the timestamp(s) are after
+  // the AHM. This is not the final check. We'll check this again after the
+  // iterators are open but there is no point in doing the work to initialize
+  // the iterators and spending the time to wait for a snapshot timestamp to be
+  // readable when we can't read back to one of the requested timestamps.
+  RETURN_NOT_OK_EVAL(VerifyLegalSnapshotTimestamps(tablet, scan_pb.read_mode(),
+                                                   tmp_snap_start_timestamp,
+                                                   tmp_snap_timestamp),
+                     *error_code = TabletServerErrorPB::INVALID_SNAPSHOT);
+
   RETURN_NOT_OK(tablet->NewRowIterator(std::move(opts), iter));
 
   // Return the picked snapshot timestamp for both READ_AT_SNAPSHOT
-  // and READ_YOUR_WRITES mode.
+  // and READ_YOUR_WRITES mode, as well as the parsed start timestamp for
+  // READ_AT_SNAPSHOT mode, if specified.
+  *snap_start_timestamp = std::move(tmp_snap_start_timestamp);
   *snap_timestamp = tmp_snap_timestamp;
   return Status::OK();
 }
@@ -2739,13 +2808,6 @@ Status TabletServiceImpl::PickAndVerifyTimestamp(const NewScanRequestPB& scan_pb
                                     scan_pb.propagated_timestamp() : Timestamp::kMin.ToUint64();
     tmp_snap_timestamp = Timestamp(std::max(propagated_timestamp + 1, clean_timestamp));
   }
-
-  // Before we wait on anything check that the timestamp is after the AHM.
-  // This is not the final check. We'll check this again after the iterators are open but
-  // there is no point in waiting if we can't actually scan afterwards.
-  RETURN_NOT_OK(VerifyNotAncientHistory(tablet,
-                                        read_mode,
-                                        tmp_snap_timestamp));
   *snap_timestamp = tmp_snap_timestamp;
   return Status::OK();
 }
diff --git a/src/kudu/tserver/tablet_service.h b/src/kudu/tserver/tablet_service.h
index c3e6d27..57622a4 100644
--- a/src/kudu/tserver/tablet_service.h
+++ b/src/kudu/tserver/tablet_service.h
@@ -27,6 +27,10 @@
 #include "kudu/tserver/tserver_admin.service.h"
 #include "kudu/tserver/tserver_service.service.h"
 
+namespace boost {
+template <class T> class optional;
+} // namespace boost
+
 namespace google {
 namespace protobuf {
 class Message;
@@ -151,12 +155,16 @@ class TabletServiceImpl : public TabletServerServiceIf {
                                    bool* has_more_results,
                                    TabletServerErrorPB::Code* error_code);
 
+  // Handle READ_AT_SNAPSHOT and READ_YOUR_WRITES scans.
+  // Returns the opened row iterator, the start timestamp of a snapshot scan,
+  // if applicable, and the ending timestamp of a scan.
   Status HandleScanAtSnapshot(const NewScanRequestPB& scan_pb,
                               const rpc::RpcContext* rpc_context,
                               const Schema& projection,
                               tablet::Tablet* tablet,
                               consensus::TimeManager* time_manager,
                               std::unique_ptr<RowwiseIterator>* iter,
+                              boost::optional<Timestamp>* snap_start_timestamp,
                               Timestamp* snap_timestamp,
                               TabletServerErrorPB::Code* error_code);
 
diff --git a/src/kudu/tserver/tserver.proto b/src/kudu/tserver/tserver.proto
index 9d7b50e..439fca0 100644
--- a/src/kudu/tserver/tserver.proto
+++ b/src/kudu/tserver/tserver.proto
@@ -75,7 +75,9 @@ message TabletServerErrorPB {
     // The tablet is hosted on this server, but not in RUNNING state.
     TABLET_NOT_RUNNING = 12;
 
-    // Client requested a snapshot read but the snapshot was invalid.
+    // Client requested a snapshot read but the snapshot was invalid. The
+    // client may be able to respond to this error by choosing a different
+    // start or end timestamp for the snapshot.
     INVALID_SNAPSHOT = 13;
 
     // An invalid scan call sequence ID was specified.
@@ -258,8 +260,31 @@ message NewScanRequestPB {
   // See common.proto for further information about read modes.
   optional ReadMode read_mode = 5 [default = READ_LATEST];
 
-  // The requested snapshot timestamp. This is only used
-  // when the read mode is set to READ_AT_SNAPSHOT.
+  // The start timestamp of a diff scan. If set, 'snap_timestamp' must also be
+  // specified and the read mode must be set to READ_AT_SNAPSHOT. Specifying
+  // this field turns the scan into a diff scan, where all changes between the
+  // start and end timestamp are returned.
+  //
+  // Diff Scans
+  // ==========
+  //
+  // When starting a diff scan, a column of type IS_DELETED (known as a virtual
+  // column) must also be included in the scan projection schema. The results
+  // of a diff scan include all values in the projection.
+  // Any row deleted between the start and end timestamps will have its
+  // IS_DELETED column set to true. In this case, any non-primary key column
+  // values in the result are undefined (they are not guaranteed to be the
+  // latest value before the row was deleted) and should be ignored. If the
+  // IS_DELETED column is set to false, then the row was inserted or updated
+  // betweeen the start and end timestamps. There is currently no way to tell
+  // whether a row returned from a diff scan with the IS_DELETED column set to
+  // false was included with the results because it was inserted or because it
+  // was updated.
+  optional fixed64 snap_start_timestamp = 16;
+
+  // The requested snapshot timestamp. This is only used when the read mode is
+  // set to READ_AT_SNAPSHOT. When 'snap_start_timestamp' is specified then
+  // this is the "end" timestamp of a diff scan.
   optional fixed64 snap_timestamp = 6;
 
   // Sent by clients which previously executed CLIENT_PROPAGATED writes.


[kudu] 02/03: tablet_service: improve error handling specificity

Posted by mp...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

mpercy pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git

commit c355e8ed77b823783d670ba7c090b12355d6340e
Author: Mike Percy <mp...@apache.org>
AuthorDate: Fri Mar 22 17:30:03 2019 -0700

    tablet_service: improve error handling specificity
    
    The error handling in TabletServiceImpl::HandleNewScanRequest() was
    convoluted. It's still not particularly inspired, but this patch makes
    several clarity and maintainability improvements:
    
    * Instead of making it the default error returned from
      HandleNewScanRequest(), localize the return of
      TabletServerErrorPB::MISMATCHED_SCHEMA to where Iterator::Init()
      returns an InvalidArgument, since that comes from a
      GetMappedReadProjection() failure.
    * Use an INVALID_SCAN_SPEC error for the case where the user specifies
      an unrecognized order mode, which is consistent with when the user
      specifies an unknown read mode or when the user specifies order mode
      ORDERED without specifying a snapshot scan. Also, update the comments
      in the .proto file to reflect the current usage.
    * Localize assignment of TabletServerErrorPB::Code as much as possible
      in HandleNewScanRequest() by plumbing an `error_code` pointer into
      HandleScanAtSnapshot() so that errors in that method can be specific
      to the type of problem observed.
    
    Change-Id: I1e6f7b01336e78ea8f37cb17b2c4485138e23581
    Reviewed-on: http://gerrit.cloudera.org:8080/12840
    Tested-by: Kudu Jenkins
    Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
 src/kudu/tools/ksck_remote-test.cc          |  4 +-
 src/kudu/tserver/tablet_server-test-base.cc | 27 +++++-----
 src/kudu/tserver/tablet_server-test-base.h  |  9 +++-
 src/kudu/tserver/tablet_server-test.cc      | 13 +++++
 src/kudu/tserver/tablet_service.cc          | 79 ++++++++++++++---------------
 src/kudu/tserver/tablet_service.h           |  3 +-
 src/kudu/tserver/tserver.proto              |  5 +-
 7 files changed, 81 insertions(+), 59 deletions(-)

diff --git a/src/kudu/tools/ksck_remote-test.cc b/src/kudu/tools/ksck_remote-test.cc
index b02c0e8..55c1ca7 100644
--- a/src/kudu/tools/ksck_remote-test.cc
+++ b/src/kudu/tools/ksck_remote-test.cc
@@ -475,8 +475,8 @@ TEST_F(RemoteKsckTest, TestChecksumSnapshotLastingLongerThanAHM) {
   ASSERT_TRUE(s.IsAborted()) << s.ToString();
   ASSERT_OK(ksck_->PrintResults());
   ASSERT_STR_CONTAINS(err_stream_.str(),
-                      "Invalid argument: Snapshot timestamp is earlier than "
-                      "the ancient history mark.");
+                      "Invalid argument: cannot verify timestamp: "
+                      "Snapshot timestamp is earlier than the ancient history mark.");
 
   // Now let's try again using the special current timestamp, which will run
   // checksums using timestamps updated from the servers, and should succeed.
diff --git a/src/kudu/tserver/tablet_server-test-base.cc b/src/kudu/tserver/tablet_server-test-base.cc
index f9bf79d..524a11a 100644
--- a/src/kudu/tserver/tablet_server-test-base.cc
+++ b/src/kudu/tserver/tablet_server-test-base.cc
@@ -433,21 +433,13 @@ void TabletServerTestBase::VerifyRows(const Schema& schema,
   ASSERT_EQ(count, expected.size());
 }
 
-// Verifies that a simple scan request fails with the specified error code/message.
-void TabletServerTestBase::VerifyScanRequestFailure(
-    const Schema& projection,
-    TabletServerErrorPB::Code expected_code,
-    const char *expected_message) {
-  ScanRequestPB req;
+void TabletServerTestBase::VerifyScanRequestFailure(const ScanRequestPB& req,
+                                                    TabletServerErrorPB::Code expected_code,
+                                                    const char *expected_message) {
   ScanResponsePB resp;
   RpcController rpc;
 
-  NewScanRequestPB* scan = req.mutable_new_scan_request();
-  scan->set_tablet_id(kTabletId);
-  ASSERT_OK(SchemaToColumnPBs(projection, scan->mutable_projected_columns()));
-  req.set_call_seq_id(0);
-
-  // Send the call
+  // Send the call.
   {
     SCOPED_TRACE(SecureDebugString(req));
     ASSERT_OK(proxy_->Scan(req, &resp, &rpc));
@@ -458,6 +450,17 @@ void TabletServerTestBase::VerifyScanRequestFailure(
   }
 }
 
+void TabletServerTestBase::VerifyScanRequestFailure(const Schema& projection,
+                                                    TabletServerErrorPB::Code expected_code,
+                                                    const char *expected_message) {
+  ScanRequestPB req;
+  NewScanRequestPB* scan = req.mutable_new_scan_request();
+  scan->set_tablet_id(kTabletId);
+  ASSERT_OK(SchemaToColumnPBs(projection, scan->mutable_projected_columns()));
+  req.set_call_seq_id(0);
+  NO_FATALS(VerifyScanRequestFailure(req, expected_code, expected_message));
+}
+
 Status TabletServerTestBase::FillNewScanRequest(ReadMode read_mode, NewScanRequestPB* scan) const {
   scan->set_tablet_id(kTabletId);
   scan->set_read_mode(read_mode);
diff --git a/src/kudu/tserver/tablet_server-test-base.h b/src/kudu/tserver/tablet_server-test-base.h
index 2f6af1d..b10e3b5 100644
--- a/src/kudu/tserver/tablet_server-test-base.h
+++ b/src/kudu/tserver/tablet_server-test-base.h
@@ -111,7 +111,14 @@ class TabletServerTestBase : public KuduTest {
   // Verifies that a set of expected rows (key, value) is present in the tablet.
   void VerifyRows(const Schema& schema, const std::vector<KeyValue>& expected);
 
-  // Verifies that a simple scan request fails with the specified error code/message.
+  // Verifies that the given scan request fails with the specified error
+  // code/message.
+  void VerifyScanRequestFailure(const ScanRequestPB& req,
+                                TabletServerErrorPB::Code expected_code,
+                                const char *expected_message);
+
+  // Verifies that a simple scan request with the given projection fails with
+  // the specified error code/message.
   void VerifyScanRequestFailure(const Schema& projection,
                                 TabletServerErrorPB::Code expected_code,
                                 const char *expected_message);
diff --git a/src/kudu/tserver/tablet_server-test.cc b/src/kudu/tserver/tablet_server-test.cc
index b720929..a4604a3 100644
--- a/src/kudu/tserver/tablet_server-test.cc
+++ b/src/kudu/tserver/tablet_server-test.cc
@@ -2471,6 +2471,19 @@ TEST_F(TabletServerTest, TestInvalidScanRequest_BadProjectionTypes) {
                            "NULLABLE found INT32 NULLABLE");
 }
 
+TEST_F(TabletServerTest, TestInvalidScanRequest_UnknownOrderMode) {
+  NO_FATALS(InsertTestRowsDirect(0, 10));
+  ScanRequestPB req;
+  NewScanRequestPB* scan = req.mutable_new_scan_request();
+  scan->set_tablet_id(kTabletId);
+  scan->set_order_mode(OrderMode::UNKNOWN_ORDER_MODE);
+  ASSERT_OK(SchemaToColumnPBs(schema_, scan->mutable_projected_columns()));
+  req.set_call_seq_id(0);
+  NO_FATALS(VerifyScanRequestFailure(req,
+                                     TabletServerErrorPB::INVALID_SCAN_SPEC,
+                                     "Unknown order mode specified"));
+}
+
 // Test that passing a projection with Column IDs throws an exception.
 // Column IDs are assigned to the user request schema on the tablet server
 // based on the latest schema.
diff --git a/src/kudu/tserver/tablet_service.cc b/src/kudu/tserver/tablet_service.cc
index 916c4c5..e710e37 100644
--- a/src/kudu/tserver/tablet_service.cc
+++ b/src/kudu/tserver/tablet_service.cc
@@ -2220,11 +2220,16 @@ Status TabletServiceImpl::HandleNewScanRequest(TabletReplica* replica,
     return Status::InvalidArgument("User requests should not have Column IDs");
   }
 
+  if (scan_pb.order_mode() == UNKNOWN_ORDER_MODE) {
+    *error_code = TabletServerErrorPB::INVALID_SCAN_SPEC;
+    return Status::InvalidArgument("Unknown order mode specified");
+  }
+
   if (scan_pb.order_mode() == ORDERED) {
     // Ordered scans must be at a snapshot so that we perform a serializable read (which can be
     // resumed). Otherwise, this would be read committed isolation, which is not resumable.
     if (scan_pb.read_mode() != READ_AT_SNAPSHOT) {
-      *error_code = TabletServerErrorPB::INVALID_SNAPSHOT;
+      *error_code = TabletServerErrorPB::INVALID_SCAN_SPEC;
           return Status::InvalidArgument("Cannot do an ordered scan that is not a snapshot read");
     }
   }
@@ -2268,8 +2273,6 @@ Status TabletServiceImpl::HandleNewScanRequest(TabletReplica* replica,
   projection = projection_builder.BuildWithoutIds();
 
   unique_ptr<RowwiseIterator> iter;
-  // Preset the error code for when creating the iterator on the tablet fails
-  TabletServerErrorPB::Code tmp_error_code = TabletServerErrorPB::MISMATCHED_SCHEMA;
 
   // It's important to keep the reference to the tablet for the case when the
   // tablet replica's shutdown is run concurrently with the code below.
@@ -2293,8 +2296,7 @@ Status TabletServiceImpl::HandleNewScanRequest(TabletReplica* replica,
     switch (scan_pb.read_mode()) {
       case UNKNOWN_READ_MODE: {
         *error_code = TabletServerErrorPB::INVALID_SCAN_SPEC;
-        s = Status::NotSupported("Unknown read mode.");
-        return s;
+        return Status::NotSupported("Unknown read mode.");
       }
       case READ_LATEST: {
         s = tablet->NewRowIterator(projection, &iter);
@@ -2304,16 +2306,8 @@ Status TabletServiceImpl::HandleNewScanRequest(TabletReplica* replica,
       case READ_AT_SNAPSHOT: {
         scoped_refptr<consensus::TimeManager> time_manager = replica->time_manager();
         s = HandleScanAtSnapshot(scan_pb, rpc_context, projection, tablet.get(),
-                                 time_manager.get(), &iter, snap_timestamp);
-        // If we got a Status::ServiceUnavailable() from HandleScanAtSnapshot() it might
-        // mean we're just behind so let the client try again.
-        if (s.IsServiceUnavailable()) {
-          *error_code = TabletServerErrorPB::THROTTLED;
-          return s;
-        }
-        if (!s.ok()) {
-          tmp_error_code = TabletServerErrorPB::INVALID_SNAPSHOT;
-        }
+                                 time_manager.get(), &iter, snap_timestamp,
+                                 error_code);
         break;
       }
     }
@@ -2329,25 +2323,27 @@ Status TabletServiceImpl::HandleNewScanRequest(TabletReplica* replica,
   if (PREDICT_TRUE(s.ok())) {
     TRACE_EVENT0("tserver", "iter->Init");
     s = iter->Init(spec.get());
+    if (PREDICT_FALSE(s.IsInvalidArgument())) {
+      // Tablet::Iterator::Init() returns InvalidArgument when an invalid
+      // projection is specified.
+      // TODO(todd): would be nice if we threaded these more specific
+      // error codes throughout Kudu.
+      *error_code = TabletServerErrorPB::MISMATCHED_SCHEMA;
+      return s;
+    }
   }
 
   TRACE("Iterator init: $0", s.ToString());
 
-  if (PREDICT_FALSE(s.IsInvalidArgument())) {
-    // An invalid projection returns InvalidArgument above.
-    // TODO: would be nice if we threaded these more specific
-    // error codes throughout Kudu.
-    *error_code = tmp_error_code;
-    return s;
-  }
   if (PREDICT_FALSE(!s.ok())) {
     LOG(WARNING) << Substitute("Error setting up scanner with request $0: $1",
                                SecureShortDebugString(*req), s.ToString());
     // If the replica has been stopped, e.g. due to disk failure, return
     // TABLET_FAILED so the scan can be handled appropriately (fail over to
     // another tablet server if fault-tolerant).
-    *error_code = tablet->HasBeenStopped() ?
-        TabletServerErrorPB::TABLET_FAILED : TabletServerErrorPB::UNKNOWN_ERROR;
+    if (tablet->HasBeenStopped()) {
+      *error_code = TabletServerErrorPB::TABLET_FAILED;
+    }
     return s;
   }
 
@@ -2373,11 +2369,8 @@ Status TabletServiceImpl::HandleNewScanRequest(TabletReplica* replica,
   // error if the snapshot timestamp was prior to the ancient history mark.
   // We have to check after we open the iterator in order to avoid a TOCTOU
   // error.
-  s = VerifyNotAncientHistory(tablet.get(), scan_pb.read_mode(), *snap_timestamp);
-  if (!s.ok()) {
-    *error_code = TabletServerErrorPB::INVALID_SNAPSHOT;
-    return s;
-  }
+  RETURN_NOT_OK_EVAL(VerifyNotAncientHistory(tablet.get(), scan_pb.read_mode(), *snap_timestamp),
+                     *error_code = TabletServerErrorPB::INVALID_SNAPSHOT);
 
   *has_more_results = iter->HasNext() && !scanner->has_fulfilled_limit();
   TRACE("has_more: $0", *has_more_results);
@@ -2607,7 +2600,8 @@ Status TabletServiceImpl::HandleScanAtSnapshot(const NewScanRequestPB& scan_pb,
                                                Tablet* tablet,
                                                consensus::TimeManager* time_manager,
                                                unique_ptr<RowwiseIterator>* iter,
-                                               Timestamp* snap_timestamp) {
+                                               Timestamp* snap_timestamp,
+                                               TabletServerErrorPB::Code* error_code) {
   switch (scan_pb.read_mode()) {
     case READ_AT_SNAPSHOT: // Fallthrough intended
     case READ_YOUR_WRITES:
@@ -2618,7 +2612,11 @@ Status TabletServiceImpl::HandleScanAtSnapshot(const NewScanRequestPB& scan_pb,
 
   // Based on the read mode, pick a timestamp and verify it.
   Timestamp tmp_snap_timestamp;
-  RETURN_NOT_OK(PickAndVerifyTimestamp(scan_pb, tablet, &tmp_snap_timestamp));
+  Status s = PickAndVerifyTimestamp(scan_pb, tablet, &tmp_snap_timestamp);
+  if (PREDICT_FALSE(!s.ok())) {
+    *error_code = TabletServerErrorPB::INVALID_SNAPSHOT;
+    return s.CloneAndPrepend("cannot verify timestamp");
+  }
 
   // Reduce the client's deadline by a few msecs to allow for overhead.
   MonoTime client_deadline = rpc_context->GetClientDeadline() - MonoDelta::FromMilliseconds(10);
@@ -2627,8 +2625,9 @@ Status TabletServiceImpl::HandleScanAtSnapshot(const NewScanRequestPB& scan_pb,
   // server will have one less available thread and the client might be stuck spending all
   // of the allotted time for the scan on a partitioned server that will never have a consistent
   // snapshot at 'snap_timestamp'.
-  // Because of this we clamp the client's deadline to the max. configured. If the client
-  // sets a long timeout then it can use it by trying in other servers.
+  // Because of this we clamp the client's deadline to the maximum configured
+  // scanner wait time. If the client sets a longer timeout then it can use it
+  // by retrying (possibly on other servers).
   bool was_clamped = false;
   MonoTime final_deadline = ClampScanDeadlineForWait(client_deadline, &was_clamped);
 
@@ -2638,19 +2637,20 @@ Status TabletServiceImpl::HandleScanAtSnapshot(const NewScanRequestPB& scan_pb,
   // the same timestamp (repeatable reads).
   TRACE("Waiting safe time to advance");
   MonoTime before = MonoTime::Now();
-  Status s = time_manager->WaitUntilSafe(tmp_snap_timestamp, final_deadline);
+  s = time_manager->WaitUntilSafe(tmp_snap_timestamp, final_deadline);
 
   tablet::MvccSnapshot snap;
-  tablet::MvccManager* mvcc_manager = tablet->mvcc_manager();
-  if (s.ok()) {
+  if (PREDICT_TRUE(s.ok())) {
     // Wait for the in-flights in the snapshot to be finished.
     TRACE("Waiting for operations to commit");
-    s = mvcc_manager->WaitForSnapshotWithAllCommitted(tmp_snap_timestamp, &snap, client_deadline);
+    s = tablet->mvcc_manager()->WaitForSnapshotWithAllCommitted(
+          tmp_snap_timestamp, &snap, client_deadline);
   }
 
   // If we got an TimeOut but we had clamped the deadline, return a ServiceUnavailable instead
   // so that the client retries.
-  if (s.IsTimedOut() && was_clamped) {
+  if (PREDICT_FALSE(s.IsTimedOut() && was_clamped)) {
+    *error_code = TabletServerErrorPB::THROTTLED;
     return Status::ServiceUnavailable(s.CloneAndPrepend(
         "could not wait for desired snapshot timestamp to be consistent").ToString());
   }
@@ -2660,9 +2660,6 @@ Status TabletServiceImpl::HandleScanAtSnapshot(const NewScanRequestPB& scan_pb,
   tablet->metrics()->snapshot_read_inflight_wait_duration->Increment(duration_usec);
   TRACE("All operations in snapshot committed. Waited for $0 microseconds", duration_usec);
 
-  if (scan_pb.order_mode() == UNKNOWN_ORDER_MODE) {
-    return Status::InvalidArgument("Unknown order mode specified");
-  }
   tablet::RowIteratorOptions opts;
   opts.projection = &projection;
   opts.snap_to_include = snap;
diff --git a/src/kudu/tserver/tablet_service.h b/src/kudu/tserver/tablet_service.h
index bc4c2b0..c3e6d27 100644
--- a/src/kudu/tserver/tablet_service.h
+++ b/src/kudu/tserver/tablet_service.h
@@ -157,7 +157,8 @@ class TabletServiceImpl : public TabletServerServiceIf {
                               tablet::Tablet* tablet,
                               consensus::TimeManager* time_manager,
                               std::unique_ptr<RowwiseIterator>* iter,
-                              Timestamp* snap_timestamp);
+                              Timestamp* snap_timestamp,
+                              TabletServerErrorPB::Code* error_code);
 
   // Validates the given timestamp is not so far in the future that
   // it exceeds the maximum allowed clock synchronization error time,
diff --git a/src/kudu/tserver/tserver.proto b/src/kudu/tserver/tserver.proto
index 3964fdb..9d7b50e 100644
--- a/src/kudu/tserver/tserver.proto
+++ b/src/kudu/tserver/tserver.proto
@@ -56,8 +56,9 @@ message TabletServerErrorPB {
     // created or has expired.
     SCANNER_EXPIRED = 7;
 
-    // An invalid scan was specified -- e.g the values passed for
-    // predicates were incorrect sizes.
+    // An invalid scan was specified. Examples of invalid scan requests include
+    // invalid or unimplemented combinations of scan options and incorrect
+    // sizes for values passed as predicates.
     INVALID_SCAN_SPEC = 8;
 
     // The provided configuration was not well-formed and/or


[kudu] 01/03: dist-test: improve error message when no tests run

Posted by mp...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

mpercy pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git

commit d533dcbe7e8c314b1551d09a115b2f0370b8bcd7
Author: Mike Percy <mp...@apache.org>
AuthorDate: Tue Mar 26 14:41:40 2019 -0700

    dist-test: improve error message when no tests run
    
    This patch improves the error message displayed when no matching tests
    are found to run.
    
    Previous error message for the "loop" action:
    
    ../build-support/dist_test.py loop --num-instances 1024 -- bin/remote_ksck-test --gtest_filter=RemoteKsckTest.TestChecksumSnapshotLastingLongerThanAHM --stress-cpu-threads=16
    isolate: at least one isolate file required
    2019-03-26 13:24:19,260 ERROR: Failed to run isolate
    Traceback (most recent call last):
      File "/home/mpercy/src/kudu/build-support/dist_test.py", line 606, in <module>
        main(sys.argv[1:])
      File "/home/mpercy/src/kudu/build-support/dist_test.py", line 601, in main
        args.func(p, args)
      File "/home/mpercy/src/kudu/build-support/dist_test.py", line 513, in loop_test
        run_isolate(staging)
      File "/home/mpercy/src/kudu/build-support/dist_test.py", line 396, in run_isolate
        '--'] + staging.gen_json_paths())
      File "/usr/local/lib/python2.7/subprocess.py", line 186, in check_call
        raise CalledProcessError(retcode, cmd)
    subprocess.CalledProcessError: Command '['isolate', 'batcharchive', '-isolate-server=http://isolate.cloudera.org:4242/', '-dump-json=/home/mpercy/src/kudu/build/isolate/dump.json', '--']' returned non-zero exit status 1
    
    New error message:
    
    ../build-support/dist_test.py loop --num-instances 1024 -- bin/remote_ksck-test --gtest_filter=RemoteKsckTest.TestChecksumSnapshotLastingLongerThanAHM --stress-cpu-threads=16
    Traceback (most recent call last):
      File "/home/mpercy/src/kudu/build-support/dist_test.py", line 608, in <module>
        main(sys.argv[1:])
      File "/home/mpercy/src/kudu/build-support/dist_test.py", line 603, in main
        args.func(p, args)
      File "/home/mpercy/src/kudu/build-support/dist_test.py", line 494, in loop_test
        raise Exception("No matching tests found for pattern %s" % tests_regex)
    Exception: No matching tests found for pattern ^remote_ksck-test(\.[0-9]+)?$
    
    Change-Id: If26dad45a8c75f773b36a1255b53b5dac006edf3
    Reviewed-on: http://gerrit.cloudera.org:8080/12865
    Tested-by: Kudu Jenkins
    Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
 build-support/dist_test.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/build-support/dist_test.py b/build-support/dist_test.py
index 01671ce..992da9c 100755
--- a/build-support/dist_test.py
+++ b/build-support/dist_test.py
@@ -434,6 +434,8 @@ def run_tests(parser, options):
   creates a task list, and submits the tasks to the testing service.
   """
   executions = get_test_executions(options.tests_regex)
+  if not executions:
+    raise Exception("No matching tests found for pattern %s" % options.tests_regex)
   if options.extra_args:
     if options.extra_args[0] == '--':
       del options.extra_args[0]
@@ -490,6 +492,8 @@ def loop_test(parser, options):
   # 'ts_location_assignment-itest'.
   tests_regex = "^" + os.path.basename(options.cmd) + "(\.[0-9]+)?$"
   executions = get_test_executions(tests_regex, options.args)
+  if not executions:
+    raise Exception("No matching tests found for pattern %s" % tests_regex)
   staging = StagingDir.new()
   # The presence of the --gtest_filter flag means the user is interested in a
   # particular subset of tests provided by the binary. In that case it doesn't