You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by wd...@apache.org on 2018/10/09 17:06:59 UTC

[1/4] kudu git commit: [docs] Add brief instructions on decommissioning tablet servers

Repository: kudu
Updated Branches:
  refs/heads/master 89206ed91 -> 9ec8d28db


[docs] Add brief instructions on decommissioning tablet servers

Change-Id: I4e9ab976390ab6c0d5b8db0da00b27dc031037e5
Reviewed-on: http://gerrit.cloudera.org:8080/11618
Tested-by: Will Berkeley <wd...@gmail.com>
Reviewed-by: Andrew Wong <aw...@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/891a8e3b
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/891a8e3b
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/891a8e3b

Branch: refs/heads/master
Commit: 891a8e3beca417007933bcae7c08067e4a3bdfb8
Parents: 89206ed
Author: Will Berkeley <wd...@gmail.org>
Authored: Mon Oct 8 14:20:45 2018 -0700
Committer: Will Berkeley <wd...@gmail.com>
Committed: Tue Oct 9 14:54:01 2018 +0000

----------------------------------------------------------------------
 docs/administration.adoc | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/891a8e3b/docs/administration.adoc
----------------------------------------------------------------------
diff --git a/docs/administration.adoc b/docs/administration.adoc
index b176f58..1c3ab20 100644
--- a/docs/administration.adoc
+++ b/docs/administration.adoc
@@ -1249,3 +1249,30 @@ table, "RF" stands for "replication factor".
 If the rebalancer is running against a cluster where rebalancing replication
 factor one tables is not supported, it will rebalance all the other tables
 and the cluster as if those singly-replicated tables did not exist.
+
+[[tablet_server_decommissioning]]
+.Decommissioning or Permanently Removing a Tablet Server From a Cluster
+
+Kudu does not currently have an automated way to remove a tablet server from
+a cluster permanently. Instead, use the following steps:
+
+. Ensure the cluster is in good health using `ksck`. See <<ksck>>.
+. If the tablet server contains any replicas of tables with replication factor
+  1, these replicas must be manually moved off the tablet server prior to
+  shutting it down. The `kudu tablet change_config move_replica` tool can be
+  used for this.
+. Shut down the tablet server. After
+  `-follower_unavailable_considered_failed_sec`, which defaults to 5 minutes,
+  Kudu will begin to re-replicate the tablet server's replicas to other servers.
+  Wait until the process is finished. Progress can be monitored using `ksck`.
+. Once all the copies are complete, `ksck` will continue to report the tablet
+  server as unavailable. The cluster will otherwise operate fine without the
+  tablet server. To completely remove it from the cluster so `ksck` shows the
+  cluster as completely healthy, restart the masters. In the case of a single
+  master, this will cause cluster downtime. With multimaster, restart the
+  masters in sequence to avoid cluster downtime.
+
+WARNING: Do not shut down multiple tablet servers at once. To remove multiple
+tablet servers from the cluster, follow the above instructions for each tablet
+server, ensuring that the previous tablet server is removed from the cluster and
+`ksck` is healthy before shutting down the next.


[4/4] kudu git commit: KUDU-2324 Add gflags to disable individual maintenance ops

Posted by wd...@apache.org.
KUDU-2324 Add gflags to disable individual maintenance ops

This patch adds one flag per maintenance op that allows the user to
individually disable each of the seven types of maintenance op. This
results in six new flags:

-enable_flush_deltamemstores
-enable_flush_memrowset
-enable_log_gc
-enable_major_delta_compaction
-enable_minor_delta_compaction
-enable_rowset_compaction

These flags do not stop the ops from being registered; instead, they
keep them from being runnable. This allows the flags to be changed at
runtime.

There was already a flag -enable_undo_delta_block_gc controlling whether
undo delta block gc was enabled; however, it prevented the
maintenance op from even being registered. The implementation of the
flag has been changed to match the others. The effect is the same,
except now the flag can be changed at runtime.

Change-Id: If4823c067883897718cc225ef85a0aaf67f1df38
Reviewed-on: http://gerrit.cloudera.org:8080/11609
Tested-by: Kudu Jenkins
Reviewed-by: Attila Bukor <ab...@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/9ec8d28d
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/9ec8d28d
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/9ec8d28d

Branch: refs/heads/master
Commit: 9ec8d28dbed301c3d0bd3d9daab183d802f6a58d
Parents: 0c91532
Author: Will Berkeley <wd...@gmail.org>
Authored: Sun Oct 7 11:07:21 2018 -0700
Committer: Will Berkeley <wd...@gmail.com>
Committed: Tue Oct 9 16:44:10 2018 +0000

----------------------------------------------------------------------
 src/kudu/tablet/tablet.cc                | 15 ++-----
 src/kudu/tablet/tablet_mm_ops.cc         | 59 +++++++++++++++++++++++++++
 src/kudu/tablet/tablet_replica_mm_ops.cc | 44 ++++++++++++++++++++
 3 files changed, 106 insertions(+), 12 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/9ec8d28d/src/kudu/tablet/tablet.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/tablet.cc b/src/kudu/tablet/tablet.cc
index ac18d41..05a024f 100644
--- a/src/kudu/tablet/tablet.cc
+++ b/src/kudu/tablet/tablet.cc
@@ -157,13 +157,6 @@ DEFINE_int32(max_encoded_key_size_bytes, 16 * 1024,
              "result in an error.");
 TAG_FLAG(max_encoded_key_size_bytes, unsafe);
 
-DEFINE_bool(enable_undo_delta_block_gc, true,
-    "Whether to enable undo delta block garbage collection. "
-    "This only affects the undo delta block deletion background task, and "
-    "doesn't control whether compactions delete ancient history. "
-    "To change what is considered ancient history use --tablet_history_max_age_sec");
-TAG_FLAG(enable_undo_delta_block_gc, evolving);
-
 METRIC_DEFINE_entity(tablet);
 METRIC_DEFINE_gauge_size(tablet, memrowset_size, "MemRowSet Memory Usage",
                          kudu::MetricUnit::kBytes,
@@ -1422,11 +1415,9 @@ void Tablet::RegisterMaintenanceOps(MaintenanceManager* maint_mgr) {
   maint_mgr->RegisterOp(major_delta_compact_op.get());
   maintenance_ops.push_back(major_delta_compact_op.release());
 
-  if (FLAGS_enable_undo_delta_block_gc) {
-    gscoped_ptr<MaintenanceOp> undo_delta_block_gc_op(new UndoDeltaBlockGCOp(this));
-    maint_mgr->RegisterOp(undo_delta_block_gc_op.get());
-    maintenance_ops.push_back(undo_delta_block_gc_op.release());
-  }
+  gscoped_ptr<MaintenanceOp> undo_delta_block_gc_op(new UndoDeltaBlockGCOp(this));
+  maint_mgr->RegisterOp(undo_delta_block_gc_op.get());
+  maintenance_ops.push_back(undo_delta_block_gc_op.release());
 
   std::lock_guard<simple_spinlock> l(state_lock_);
   maintenance_ops_.swap(maintenance_ops);

http://git-wip-us.apache.org/repos/asf/kudu/blob/9ec8d28d/src/kudu/tablet/tablet_mm_ops.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/tablet_mm_ops.cc b/src/kudu/tablet/tablet_mm_ops.cc
index 3af1042..c71bc6e 100644
--- a/src/kudu/tablet/tablet_mm_ops.cc
+++ b/src/kudu/tablet/tablet_mm_ops.cc
@@ -18,6 +18,7 @@
 #include "kudu/tablet/tablet_mm_ops.h"
 
 #include <mutex>
+#include <ostream>
 #include <utility>
 
 #include <gflags/gflags.h>
@@ -41,6 +42,36 @@ DEFINE_int32(undo_delta_block_gc_init_budget_millis, 1000,
 TAG_FLAG(undo_delta_block_gc_init_budget_millis, evolving);
 TAG_FLAG(undo_delta_block_gc_init_budget_millis, advanced);
 
+DEFINE_bool(enable_major_delta_compaction, true,
+    "Whether to enable major delta compaction. Disabling major delta "
+    "compaction may worsen performance and increase disk space usage for "
+    "workloads involving updates and deletes.");
+TAG_FLAG(enable_major_delta_compaction, runtime);
+TAG_FLAG(enable_major_delta_compaction, unsafe);
+
+DEFINE_bool(enable_minor_delta_compaction, true,
+    "Whether to enable minor delta compaction. Disabling minor delta "
+    "compaction may worsen performance and increase disk space usage for "
+    "workloads involving updates and deletes.");
+TAG_FLAG(enable_minor_delta_compaction, runtime);
+TAG_FLAG(enable_minor_delta_compaction, unsafe);
+
+DEFINE_bool(enable_rowset_compaction, true,
+    "Whether to enable rowset compaction. Disabling rowset compaction "
+    "may worsen performance and increase disk space usage.");
+TAG_FLAG(enable_rowset_compaction, runtime);
+TAG_FLAG(enable_rowset_compaction, unsafe);
+
+DEFINE_bool(enable_undo_delta_block_gc, true,
+    "Whether to enable undo delta block garbage collection. Disabling undo "
+    "delta block garbage collection may worsen performance and increase disk "
+    "space usage for workloads involving updates and deletes. This only "
+    "affects the undo delta block deletion background task, and doesn't "
+    "control whether compactions delete ancient history. To change what is "
+    "considered ancient history use --tablet_history_max_age_sec");
+TAG_FLAG(enable_undo_delta_block_gc, runtime);
+TAG_FLAG(enable_undo_delta_block_gc, unsafe);
+
 using std::string;
 using strings::Substitute;
 
@@ -68,6 +99,13 @@ CompactRowSetsOp::CompactRowSetsOp(Tablet* tablet)
 }
 
 void CompactRowSetsOp::UpdateStats(MaintenanceOpStats* stats) {
+  if (PREDICT_FALSE(!FLAGS_enable_rowset_compaction)) {
+    KLOG_EVERY_N_SECS(WARNING, 300)
+        << "Rowset compaction is disabled (check --enable_rowset_compaction)";
+    stats->set_runnable(false);
+    return;
+  }
+
   std::lock_guard<simple_spinlock> l(lock_);
 
   // Any operation that changes the on-disk row layout invalidates the
@@ -132,6 +170,13 @@ MinorDeltaCompactionOp::MinorDeltaCompactionOp(Tablet* tablet)
 }
 
 void MinorDeltaCompactionOp::UpdateStats(MaintenanceOpStats* stats) {
+  if (PREDICT_FALSE(!FLAGS_enable_minor_delta_compaction)) {
+    KLOG_EVERY_N_SECS(WARNING, 300)
+        << "Minor delta compaction is disabled (check --enable_minor_delta_compaction)";
+    stats->set_runnable(false);
+    return;
+  }
+
   std::lock_guard<simple_spinlock> l(lock_);
 
   // Any operation that changes the number of REDO files invalidates the
@@ -204,6 +249,13 @@ MajorDeltaCompactionOp::MajorDeltaCompactionOp(Tablet* tablet)
 }
 
 void MajorDeltaCompactionOp::UpdateStats(MaintenanceOpStats* stats) {
+  if (PREDICT_FALSE(!FLAGS_enable_major_delta_compaction)) {
+    KLOG_EVERY_N_SECS(WARNING, 300)
+        << "Major delta compaction is disabled (check --enable_major_delta_compaction)";
+    stats->set_runnable(false);
+    return;
+  }
+
   std::lock_guard<simple_spinlock> l(lock_);
 
   // Any operation that changes the size of the on-disk data invalidates the
@@ -275,6 +327,13 @@ UndoDeltaBlockGCOp::UndoDeltaBlockGCOp(Tablet* tablet)
 }
 
 void UndoDeltaBlockGCOp::UpdateStats(MaintenanceOpStats* stats) {
+  if (PREDICT_FALSE(!FLAGS_enable_undo_delta_block_gc)) {
+    KLOG_EVERY_N_SECS(WARNING, 300)
+        << "Undo delta block GC is disabled (check --enable_undo_delta_block_gc)";
+    stats->set_runnable(false);
+    return;
+  }
+
   int64_t max_estimated_retained_bytes = 0;
   WARN_NOT_OK(tablet_->EstimateBytesInPotentiallyAncientUndoDeltas(&max_estimated_retained_bytes),
               "Unable to count bytes in potentially ancient undo deltas");

http://git-wip-us.apache.org/repos/asf/kudu/blob/9ec8d28d/src/kudu/tablet/tablet_replica_mm_ops.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/tablet_replica_mm_ops.cc b/src/kudu/tablet/tablet_replica_mm_ops.cc
index b3761a4..b16fae3 100644
--- a/src/kudu/tablet/tablet_replica_mm_ops.cc
+++ b/src/kudu/tablet/tablet_replica_mm_ops.cc
@@ -29,11 +29,34 @@
 #include "kudu/gutil/strings/substitute.h"
 #include "kudu/tablet/tablet_metrics.h"
 #include "kudu/util/flag_tags.h"
+#include "kudu/util/logging.h"
 #include "kudu/util/maintenance_manager.h"
 #include "kudu/util/metrics.h"
 #include "kudu/util/scoped_cleanup.h"
 #include "kudu/util/status.h"
 
+DEFINE_bool(enable_flush_memrowset, true,
+    "Whether to enable memrowset flush. Disabling memrowset flush prevents "
+    "the tablet server from flushing writes to diskrowsets, resulting in "
+    "increasing memory and WAL disk space usage.");
+TAG_FLAG(enable_flush_memrowset, runtime);
+TAG_FLAG(enable_flush_memrowset, unsafe);
+
+DEFINE_bool(enable_flush_deltamemstores, true,
+    "Whether to enable deltamemstore flush. Disabling deltamemstore flush "
+    "prevents the tablet server from flushing updates to deltafiles, resulting "
+    "in increasing memory and WAL disk space usage for workloads involving "
+    "updates and deletes.");
+TAG_FLAG(enable_flush_deltamemstores, runtime);
+TAG_FLAG(enable_flush_deltamemstores, unsafe);
+
+DEFINE_bool(enable_log_gc, true,
+    "Whether to enable write-ahead log garbage collection. Disabling WAL "
+    "garbage collection will cause the tablet server to stop reclaiming space "
+    "from the WAL, leading to increasing WAL disk space usage.");
+TAG_FLAG(enable_log_gc, runtime);
+TAG_FLAG(enable_log_gc, unsafe);
+
 DEFINE_int32(flush_threshold_mb, 1024,
              "Size at which MemRowSet flushes are triggered. "
              "A MRS can still flush below this threshold if it if hasn't flushed in a while, "
@@ -100,6 +123,13 @@ void FlushOpPerfImprovementPolicy::SetPerfImprovementForFlush(MaintenanceOpStats
 //
 
 void FlushMRSOp::UpdateStats(MaintenanceOpStats* stats) {
+  if (PREDICT_FALSE(!FLAGS_enable_flush_memrowset)) {
+    KLOG_EVERY_N_SECS(WARNING, 300)
+        << "Memrowset flush is disabled (check --enable_flush_memrowset)";
+    stats->set_runnable(false);
+    return;
+  }
+
   std::lock_guard<simple_spinlock> l(lock_);
 
   map<int64_t, int64_t> replay_size_map;
@@ -166,6 +196,13 @@ scoped_refptr<AtomicGauge<uint32_t> > FlushMRSOp::RunningGauge() const {
 //
 
 void FlushDeltaMemStoresOp::UpdateStats(MaintenanceOpStats* stats) {
+  if (PREDICT_FALSE(!FLAGS_enable_flush_deltamemstores)) {
+    KLOG_EVERY_N_SECS(WARNING, 300)
+        << "Deltamemstore flush is disabled (check --enable_flush_deltamemstores)";
+    stats->set_runnable(false);
+    return;
+  }
+
   std::lock_guard<simple_spinlock> l(lock_);
   int64_t dms_size;
   int64_t retention_size;
@@ -230,6 +267,13 @@ LogGCOp::LogGCOp(TabletReplica* tablet_replica)
       sem_(1) {}
 
 void LogGCOp::UpdateStats(MaintenanceOpStats* stats) {
+  if (PREDICT_FALSE(!FLAGS_enable_log_gc)) {
+    KLOG_EVERY_N_SECS(WARNING, 300)
+        << "Log GC is disabled (check --enable_log_gc)";
+    stats->set_runnable(false);
+    return;
+  }
+
   int64_t retention_size;
 
   if (!tablet_replica_->GetGCableDataSize(&retention_size).ok()) {


[2/4] kudu git commit: Cleanup in rowset SVG layout dumping

Posted by wd...@apache.org.
Cleanup in rowset SVG layout dumping

I tested this by manually verifying that the /tablet-rowsetlayout-svg
endpoint and rowset layout SVG file dumping worked as expected.

Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
Reviewed-on: http://gerrit.cloudera.org:8080/11612
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Attila Bukor <ab...@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/77ead354
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/77ead354
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/77ead354

Branch: refs/heads/master
Commit: 77ead354b6ac86e3d251e35d9b0cf9da62979054
Parents: 891a8e3
Author: Will Berkeley <wd...@gmail.org>
Authored: Sun Oct 7 21:56:14 2018 -0700
Committer: Will Berkeley <wd...@gmail.com>
Committed: Tue Oct 9 15:10:20 2018 +0000

----------------------------------------------------------------------
 src/kudu/tablet/compaction_policy.cc |   2 +-
 src/kudu/tablet/svg_dump.cc          | 172 ++++++++++++++----------------
 src/kudu/tablet/svg_dump.h           |  23 ++--
 src/kudu/tablet/tablet.cc            |   2 +-
 4 files changed, 95 insertions(+), 104 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/77ead354/src/kudu/tablet/compaction_policy.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/compaction_policy.cc b/src/kudu/tablet/compaction_policy.cc
index 745f28b..d1ead15 100644
--- a/src/kudu/tablet/compaction_policy.cc
+++ b/src/kudu/tablet/compaction_policy.cc
@@ -471,7 +471,7 @@ Status BudgetedCompactionPolicy::PickRowSets(const RowSetTree &tree,
   }
 
   picked->swap(best_solution.rowsets);
-  DumpCompactionSVG(asc_min_key, *picked);
+  DumpCompactionSVGToFile(asc_min_key, *picked);
 
   return Status::OK();
 }

http://git-wip-us.apache.org/repos/asf/kudu/blob/77ead354/src/kudu/tablet/svg_dump.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/svg_dump.cc b/src/kudu/tablet/svg_dump.cc
index 1427b9b..9c88655 100644
--- a/src/kudu/tablet/svg_dump.cc
+++ b/src/kudu/tablet/svg_dump.cc
@@ -17,8 +17,8 @@
 
 #include "kudu/tablet/svg_dump.h"
 
+#include <algorithm>
 #include <ctime>
-#include <fstream>
 #include <string>
 #include <unordered_set>
 #include <vector>
@@ -26,13 +26,16 @@
 #include <gflags/gflags.h>
 #include <glog/logging.h>
 
-#include "kudu/gutil/gscoped_ptr.h"
 #include "kudu/gutil/macros.h"
 #include "kudu/gutil/map-util.h"
 #include "kudu/gutil/stringprintf.h"
+#include "kudu/gutil/strings/substitute.h"
 #include "kudu/gutil/strings/util.h"
 #include "kudu/tablet/rowset_info.h"
+#include "kudu/util/env.h"
 #include "kudu/util/flag_tags.h"
+#include "kudu/util/slice.h"
+#include "kudu/util/status.h"
 
 // Flag to dump SVGs of every compaction decision.
 //
@@ -40,7 +43,6 @@
 // commands like:
 // $ for x in compaction-*svg ; do convert $x $x.png ; done
 // $ mencoder mf://compaction*png -mf fps=1 -ovc lavc -o compactions.avi
-
 DEFINE_string(compaction_policy_dump_svgs_pattern, "",
               "File path into which to dump SVG visualization of "
               "selected compactions. This is mostly useful in "
@@ -49,10 +51,12 @@ DEFINE_string(compaction_policy_dump_svgs_pattern, "",
               "with the compaction selection timestamp.");
 TAG_FLAG(compaction_policy_dump_svgs_pattern, hidden);
 
+using std::endl;
 using std::ostream;
 using std::string;
 using std::unordered_set;
 using std::vector;
+using strings::Substitute;
 
 namespace kudu {
 namespace tablet {
@@ -63,35 +67,30 @@ namespace {
 // distributes 'rowsets' into separate vectors in 'rows' such that
 // within any given row, none of the rowsets overlap in keyspace.
 void OrganizeSVGRows(const vector<RowSetInfo>& candidates,
-                     vector<vector<const RowSetInfo*> >* rows) {
-  rows->push_back(vector<const RowSetInfo *>());
-
-  for (const RowSetInfo &candidate : candidates) {
-    // Slot into the first row of the output which fits it
+                     vector<vector<const RowSetInfo*>>* rows) {
+  DCHECK(rows);
+  rows->clear();
+  for (const auto& candidate : candidates) {
+    // Slot into the first row which fits it.
     bool found_slot = false;
-    for (vector<const RowSetInfo *> &row : *rows) {
-      // If this candidate doesn't intersect any other candidates in this
-      // row, we can put it here.
-      bool fits_in_row = true;
-      for (const RowSetInfo *already_in_row : row) {
-        if (candidate.Intersects(*already_in_row)) {
-          fits_in_row = false;
-          break;
-        }
-      }
+    for (auto& row : *rows) {
+      // If this candidate doesn't intersect any other rowsets already in this
+      // row, we can put it in this row.
+      auto fits_in_row = std::none_of(row.begin(),
+                                      row.end(),
+                                      [&candidate](const RowSetInfo* already_in_row) {
+                                        return candidate.Intersects(*already_in_row);
+                                      });
       if (fits_in_row) {
         row.push_back(&candidate);
         found_slot = true;
         break;
       }
     }
-
     // If we couldn't find a spot in any existing row, add a new row
     // to the bottom of the SVG.
     if (!found_slot) {
-      vector<const RowSetInfo *> new_row;
-      new_row.push_back(&candidate);
-      rows->push_back(new_row);
+      rows->push_back({ &candidate });
     }
   }
 }
@@ -99,79 +98,62 @@ void OrganizeSVGRows(const vector<RowSetInfo>& candidates,
 void DumpSVG(const vector<RowSetInfo>& candidates,
              const unordered_set<RowSet*>& picked,
              ostream* outptr) {
-  CHECK(outptr) << "Dump SVG expects an ostream";
-  CHECK(outptr->good()) << "Dump SVG expects a good ostream";
-  using std::endl;
+  CHECK(outptr);
+  CHECK(outptr->good());
   ostream& out = *outptr;
 
-  vector<vector<const RowSetInfo*> > svg_rows;
+  vector<vector<const RowSetInfo*>> svg_rows;
   OrganizeSVGRows(candidates, &svg_rows);
 
-  const char *kPickedColor = "#f66";
-  const char *kDefaultColor = "#666";
-  const double kTotalWidth = 1200;
-  const int kRowHeight = 15;
-  const double kHeaderHeight = 60;
+  const char *kPickedColor = "#f66"; // Light red.
+  const char *kDefaultColor = "#666"; // Dark gray.
+  constexpr double kTotalWidth = 1200.0;
+  constexpr int kRowHeight = 15;
+  constexpr double kHeaderHeight = 60.0;
   const double kTotalHeight = kRowHeight * svg_rows.size() + kHeaderHeight;
 
-  out << "<svg version=\"1.1\" width=\"" << kTotalWidth << "\" height=\""
-      << kTotalHeight << "\""
-      << " viewBox=\"0 0 " << kTotalWidth << " " << kTotalHeight << "\""
-      << " xmlns=\"http://www.w3.org/2000/svg\" >" << endl;
+  out << Substitute(
+      R"(<svg version="1.1" width="$0" height="$1" viewBox="0 0 $0 $1" )"
+      R"(xmlns="http://www.w3.org/2000/svg">)",
+      kTotalWidth, kTotalHeight)
+      << endl;
 
-  // Background
-  out << "<rect x=\"0.0\" y=\"0\" width=\"1200.0\" height=\"" << kTotalHeight << "\""
-      << " fill=\"#fff\" />" << endl;
+  // Background.
+  out << Substitute(R"(<rect x="0" y="0" width="$0" height="$1" fill="#fff"/>)",
+                    kTotalWidth, kTotalHeight)
+      << endl;
 
-  for (int row_index = 0; row_index < svg_rows.size(); row_index++) {
+  for (auto row_index = 0; row_index < svg_rows.size(); row_index++) {
     const vector<const RowSetInfo *> &row = svg_rows[row_index];
 
-    int y = kRowHeight * row_index + kHeaderHeight;
+    const auto y = kRowHeight * row_index + kHeaderHeight;
     for (const RowSetInfo *cand : row) {
-      bool was_picked = ContainsKey(picked, cand->rowset());
-      const char *color = was_picked ? kPickedColor : kDefaultColor;
-
-      double x = cand->cdf_min_key() * kTotalWidth;
-      double width = cand->width() * kTotalWidth;
-      out << StringPrintf("<rect x=\"%f\" y=\"%d\" width=\"%f\" height=\"%d\" "
-                          "stroke=\"#000\" fill=\"%s\"/>",
-                          x, y, width, kRowHeight, color) << endl;
-      out << StringPrintf("<text x=\"%f\" y=\"%d\" width=\"%f\" height=\"%d\" "
-                          "fill=\"rgb(0,0,0)\">%dMB</text>",
-                          x, y + kRowHeight, width, kRowHeight, cand->size_mb()) << endl;
+      const char *color = ContainsKey(picked, cand->rowset()) ? kPickedColor :
+                                                                kDefaultColor;
+      const auto x = cand->cdf_min_key() * kTotalWidth;
+      const auto width = cand->width() * kTotalWidth;
+      out << Substitute(
+          R"(<rect x="$0" y="$1" width="$2" height="$3" stroke="#000" fill="$4"/>)",
+          x, y, width, kRowHeight, color)
+          << endl;
+      out << Substitute(R"+(<text x="$0" y="$1" width="$2" height="$3" )+"
+                        R"+(fill="rgb(0,0,0)">$4MB</text>)+",
+                        x, y + kRowHeight, width, kRowHeight, cand->size_mb())
+          << endl;
     }
   }
 
   out << "</svg>" << endl;
 }
 
-void PrintXMLHeader(ostream* o) {
-  CHECK(o) << "XML header printer expects an ostream";
-  CHECK(o->good()) << "XML header printer expects a good ostream";
-  *o << "<?xml version=\"1.0\" standalone=\"no\"?>" << std::endl;
-  *o << "<!DOCTYPE svg PUBLIC \"-//W3C//DTD SVG 1.1//EN\" "
-     << "\"http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd\">" << std::endl;
-}
-
-// Prepares ofstream to default dump location.
-// In case any of the preparation fails or default pattern is empty,
-// NULL is returned.
-gscoped_ptr<ostream> PrepareOstream() {
-  using std::ofstream;
-  gscoped_ptr<ofstream> out;
-  // Get default file name
-  const string &pattern = FLAGS_compaction_policy_dump_svgs_pattern;
-  if (pattern.empty()) return gscoped_ptr<ostream>();
-  const string path = StringReplace(pattern, "TIME", StringPrintf("%ld", time(nullptr)), true);
-
-  // Open
-  out.reset(new ofstream(path.c_str()));
-  if (!out->is_open()) {
-    LOG(WARNING) << "Could not dump compaction output to " << path << ": file open failed";
-    return gscoped_ptr<ostream>();
-  }
-
-  return out.PassAs<ostream>();
+void PrintXMLHeader(ostream* out) {
+  CHECK(out);
+  CHECK(out->good());
+  *out << R"(<?xml version="1.0" standalone="no"?>)"
+       << endl
+       << R"(<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" )"
+       << R"("http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">)"
+       << endl;
 }
 
 } // anonymous namespace
@@ -179,23 +161,31 @@ gscoped_ptr<ostream> PrepareOstream() {
 void DumpCompactionSVG(const vector<RowSetInfo>& candidates,
                        const unordered_set<RowSet*>& picked,
                        ostream* out,
-                       bool print_xml) {
-  // Get the desired pointer to the ostream
-  gscoped_ptr<ostream> dfl;
-  if (!out) {
-    dfl = PrepareOstream();
-    out = dfl.get();
-    if (!out) return;
-  }
-
-  // Print out with the correct ostream
-  LOG(INFO) << "Dumping SVG of DiskRowSetLayout with"
-            << (print_xml ? "" : "out") << " XML header";
-  if (print_xml) {
+                       bool print_xml_header) {
+  CHECK(out);
+  VLOG(1) << "Dumping SVG of DiskRowSetLayout with"
+          << (print_xml_header ? "" : "out") << " XML header";
+  if (print_xml_header) {
     PrintXMLHeader(out);
   }
   DumpSVG(candidates, picked, out);
 }
 
+void DumpCompactionSVGToFile(const vector<RowSetInfo>& candidates,
+                             const unordered_set<RowSet*>& picked) {
+  const string& pattern = FLAGS_compaction_policy_dump_svgs_pattern;
+  if (pattern.empty()) {
+    return;
+  }
+  const string path = StringReplace(pattern,
+                                    "TIME",
+                                    StringPrintf("%ld", time(nullptr)),
+                                    /*replace_all=*/true);
+  std::ostringstream buf;
+  DumpCompactionSVG(candidates, picked, &buf, /*print_xml_header=*/true);
+  WARN_NOT_OK(WriteStringToFile(Env::Default(), buf.str(), path),
+              "unable to dump rowset compaction SVG to file");
+}
+
 } // namespace tablet
 } // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/77ead354/src/kudu/tablet/svg_dump.h
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/svg_dump.h b/src/kudu/tablet/svg_dump.h
index 59c1397..706f573 100644
--- a/src/kudu/tablet/svg_dump.h
+++ b/src/kudu/tablet/svg_dump.h
@@ -17,7 +17,6 @@
 #ifndef KUDU_TABLET_COMPACTION_SVG_DUMP_H_
 #define KUDU_TABLET_COMPACTION_SVG_DUMP_H_
 
-#include <cstddef>
 #include <ostream>
 #include <unordered_set>
 #include <vector>
@@ -26,20 +25,22 @@ namespace kudu {
 namespace tablet {
 
 class RowSet;
-
 class RowSetInfo;
 
-// Dump an SVG file which represents the candidates
-// for compaction, highlighting the ones that were selected.
-// Dumps in to parameter ostream. If ostream is null, then default ostream
-// specified as a flag is used (see svg_dump.cc).
-// The last optional parameter controls whether to print an XML header in
-// the file. If true, prints the header (xml tag and DOCTYPE). Otherwise, only
-// the <svg>...</svg> section is printed.
+// Dumps an SVG file which describes the rowset layout for the tablet replica
+// and which highlights rowsets that would be selected for the next rowset
+// compaction, if one were run. The SVG is printed to 'out', which must not be
+// null. If 'print_xml_header' is true, prints an XML header including the xml
+// tag and DOCTYPE. Otherwise, only the '<svg>...</svg>' section is printed.
 void DumpCompactionSVG(const std::vector<RowSetInfo>& candidates,
                        const std::unordered_set<RowSet*>& picked,
-                       std::ostream* out = NULL,
-                       bool print_xml = true);
+                       std::ostream* out,
+                       bool print_xml_header);
+
+// Like the above, but dumps the SVG to a file named according to the rules of
+// --compaction_policy_dump_svgs_pattern. See the flag definition in svg_dump.cc.
+void DumpCompactionSVGToFile(const std::vector<RowSetInfo>& candidates,
+                             const std::unordered_set<RowSet*>& picked);
 
 } // namespace tablet
 } // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/77ead354/src/kudu/tablet/tablet.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/tablet.cc b/src/kudu/tablet/tablet.cc
index 4fa272f..ac18d41 100644
--- a/src/kudu/tablet/tablet.cc
+++ b/src/kudu/tablet/tablet.cc
@@ -2306,7 +2306,7 @@ void Tablet::PrintRSLayout(ostream* o) {
 
   vector<RowSetInfo> min, max;
   RowSetInfo::CollectOrdered(*rowsets_copy, &min, &max);
-  DumpCompactionSVG(min, picked, o, false);
+  DumpCompactionSVG(min, picked, o, /*print_xml_header=*/false);
 
   *o << "<h2>Compaction policy log</h2>" << std::endl;
 


[3/4] kudu git commit: [compaction] Small improvements to compaction policy tests

Posted by wd...@apache.org.
[compaction] Small improvements to compaction policy tests

To learn more about compaction policy, I spent some time reading over
compaction_policy-test.cc, and in doing so I made a bunch of small
improvements. I also added one test covering a facet of the policy
that's pretty important but isn't obviously covered in the design docs
or the existing tests.

There are no functional non-test changes in this patch. I added
scaled-down versions of a couple of slow tests that can run all the
time, and made one slow test only run when slow tests are allowed.

Change-Id: I8f7a0e2c69f301d7fc7ca2fac657569ea240d4e3
Reviewed-on: http://gerrit.cloudera.org:8080/11605
Tested-by: Kudu Jenkins
Reviewed-by: Attila Bukor <ab...@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/0c91532e
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/0c91532e
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/0c91532e

Branch: refs/heads/master
Commit: 0c91532e742df3b8f09d94ede5d7cde701b3857c
Parents: 77ead35
Author: Will Berkeley <wd...@gmail.org>
Authored: Fri Oct 5 20:41:19 2018 -0700
Committer: Will Berkeley <wd...@gmail.com>
Committed: Tue Oct 9 16:44:00 2018 +0000

----------------------------------------------------------------------
 src/kudu/tablet/compaction_policy-test.cc | 196 +++++++++++++++++--------
 1 file changed, 131 insertions(+), 65 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/0c91532e/src/kudu/tablet/compaction_policy-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/compaction_policy-test.cc b/src/kudu/tablet/compaction_policy-test.cc
index ddf917f..4277f9d 100644
--- a/src/kudu/tablet/compaction_policy-test.cc
+++ b/src/kudu/tablet/compaction_policy-test.cc
@@ -60,124 +60,188 @@ class TestCompactionPolicy : public KuduTest {
 
     BudgetedCompactionPolicy policy(size_budget_mb);
 
-    ASSERT_OK(policy.PickRowSets(tree, picked, quality, nullptr));
+    ASSERT_OK(policy.PickRowSets(tree, picked, quality, /*log=*/nullptr));
   }
 };
 
 
 // Simple test for budgeted compaction: with three rowsets which
-// mostly overlap, and an high budget, they should all be selected.
+// mostly overlap, and a high budget, they should all be selected.
 TEST_F(TestCompactionPolicy, TestBudgetedSelection) {
-  RowSetVector vec = {
+  /*
+   *   [C ------ c]
+   *  [B ----- a]
+   * [A ------- b]
+   */
+  const RowSetVector rowsets = {
     std::make_shared<MockDiskRowSet>("C", "c"),
     std::make_shared<MockDiskRowSet>("B", "a"),
     std::make_shared<MockDiskRowSet>("A", "b")
   };
 
-  const int kBudgetMb = 1000; // enough to select all
+  constexpr auto kBudgetMb = 1000; // Enough to select all rowsets.
   unordered_set<RowSet*> picked;
-  double quality = 0;
-  NO_FATALS(RunTestCase(vec, kBudgetMb, &picked, &quality));
-  ASSERT_EQ(3, picked.size());
+  double quality = 0.0;
+  NO_FATALS(RunTestCase(rowsets, kBudgetMb, &picked, &quality));
+  ASSERT_EQ(rowsets.size(), picked.size());
+  // Since all three rowsets are picked and each covers almost the entire key
+  // range, the sum of widths is about 3 while the union width is 1, so the
+  // quality score should be between 1 and 2.
   ASSERT_GE(quality, 1.0);
+  ASSERT_LE(quality, 2.0);
 }
 
 // Test for the case when we have many rowsets, but none of them
 // overlap at all. This is likely to occur in workloads where the
 // primary key is always increasing (such as a timestamp).
-TEST_F(TestCompactionPolicy, TestNonOverlappingRowsets) {
-  if (!AllowSlowTests()) {
-    LOG(INFO) << "Skipped";
-    return;
-  }
-  RowSetVector vec;
-  for (int i = 0; i < 10000;  i++) {
-    vec.emplace_back(new MockDiskRowSet(
+TEST_F(TestCompactionPolicy, TestNonOverlappingRowSets) {
+  /* NB: Zero-padding of string keys omitted to save space.
+   *
+   * [0 - 1] [2 - 3] ... [198 - 199]
+   */
+  const auto kNumRowSets = AllowSlowTests() ? 10000 : 100;
+  RowSetVector rowsets;
+  for (auto i = 0; i < kNumRowSets; i++) {
+    rowsets.emplace_back(new MockDiskRowSet(
         StringPrintf("%010d", i * 2),
         StringPrintf("%010d", i * 2 + 1)));
   }
+  constexpr auto kBudgetMb = 128;
   unordered_set<RowSet*> picked;
-  double quality = 0;
-  NO_FATALS(RunTestCase(vec, /*size_budget_mb=*/128, &picked, &quality));
-  ASSERT_EQ(quality, 0);
+  double quality = 0.0;
+  NO_FATALS(RunTestCase(rowsets, kBudgetMb, &picked, &quality));
+  ASSERT_EQ(quality, 0.0);
   ASSERT_TRUE(picked.empty());
 }
 
 // Similar to the above, but with some small overlap between adjacent
 // rowsets.
-TEST_F(TestCompactionPolicy, TestTinyOverlapRowsets) {
-  if (!AllowSlowTests()) {
-    LOG(INFO) << "Skipped";
-    return;
-  }
-
-  RowSetVector vec;
-  for (int i = 0; i < 10000;  i++) {
-    vec.emplace_back(new MockDiskRowSet(
-        StringPrintf("%010d", i * 10000),
-        StringPrintf("%010d", i * 10000 + 11000)));
+TEST_F(TestCompactionPolicy, TestTinyOverlapRowSets) {
+  /* NB: Zero-padding of string keys omitted to save space.
+   *
+   * [0 - 11000]
+   *    [10000 - 21000]
+   *           ...
+   *                    [990000 - 1001000]
+   */
+  const auto kNumRowSets = AllowSlowTests() ? 10000 : 100;
+  RowSetVector rowsets;
+  for (auto i = 0; i < kNumRowSets; i++) {
+    rowsets.emplace_back(new MockDiskRowSet(
+        StringPrintf("%09d", i * 10000),
+        StringPrintf("%09d", i * 10000 + 11000)));
   }
+  constexpr auto kBudgetMb = 128;
   unordered_set<RowSet*> picked;
   double quality = 0;
-  NO_FATALS(RunTestCase(vec, /*size_budget_mb=*/128, &picked, &quality));
-  ASSERT_EQ(quality, 0);
+  NO_FATALS(RunTestCase(rowsets, kBudgetMb, &picked, &quality));
+  // With such small overlaps, no compaction will be considered worthwhile.
+  ASSERT_EQ(quality, 0.0);
   ASSERT_TRUE(picked.empty());
 }
 
 // Test case with 100 rowsets, each of which overlaps with its two
 // neighbors to the right.
 TEST_F(TestCompactionPolicy, TestSignificantOverlap) {
-  RowSetVector vec;
-  for (int i = 0; i < 100;  i++) {
-    vec.emplace_back(new MockDiskRowSet(
+  /* NB: Zero-padding of string keys omitted to save space.
+   *
+   * [0 ------ 20000]
+   *    [10000 ------ 30000]
+   *           [20000 ------ 40000]
+   *                  ...
+   *                                [990000 - 1010000]
+   */
+  constexpr auto kNumRowSets = 100;
+  RowSetVector rowsets;
+  for (auto i = 0; i < kNumRowSets;  i++) {
+    rowsets.emplace_back(new MockDiskRowSet(
         StringPrintf("%010d", i * 10000),
         StringPrintf("%010d", (i + 2) * 10000)));
   }
+  constexpr auto kBudgetMb = 64;
   unordered_set<RowSet*> picked;
-  double quality = 0;
-  const int kBudgetMb = 64;
-  NO_FATALS(RunTestCase(vec, kBudgetMb, &picked, &quality));
-  ASSERT_GT(quality, 0.5);
-  // Each rowset is 1MB so we should exactly fill the budget.
+  double quality = 0.0;
+  NO_FATALS(RunTestCase(rowsets, kBudgetMb, &picked, &quality));
+  // Each rowset is 1MB so the number of rowsets picked should be the number of
+  // MB in the budget.
   ASSERT_EQ(picked.size(), kBudgetMb);
+
+  // In picking 64 of 100 equally-sized and -spaced rowsets, the union width is
+  // about 0.64. Each rowset intersects the next in half its width, which makes
+  // the sum of widths about 2 * 0.64 = 1.28, if we ignore the smaller overlap
+  // between the ith and (i + 2)th rowsets. So, the quality score should be
+  // around 0.64.
+  ASSERT_GT(quality, 0.5);
+}
+
+// Test the adjustment we make to the quality measure that penalizes wider
+// solutions that have almost the same quality score. For example, with no
+// adjustment and inputs
+//
+//  [ -- A -- ]
+//  [ -- B -- ]
+//            [ -- C -- ]
+//
+// compacting {A, B, C} results in the same quality score as {A, B}, 0.67, but
+// uses more budget. By penalizing the wider solution {A, B, C}, we ensure we
+// don't waste I/O.
+TEST_F(TestCompactionPolicy, TestSupportAdjust) {
+  const RowSetVector rowsets = {
+    std::make_shared<MockDiskRowSet>("A", "B"),
+    std::make_shared<MockDiskRowSet>("A", "B"),
+    std::make_shared<MockDiskRowSet>("B", "C"),
+  };
+  constexpr auto kBudgetMb = 1000; // Enough to select all rowsets.
+  unordered_set<RowSet*> picked;
+  double quality = 0.0;
+  NO_FATALS(RunTestCase(rowsets, kBudgetMb, &picked, &quality));
+  ASSERT_EQ(2, picked.size());
+  // The weights of A and B are both 0.67, so with the adjustment the quality
+  // score should be a little less than 0.67.
+  ASSERT_GE(quality, 0.6);
 }
 
 static RowSetVector LoadFile(const string& name) {
-  RowSetVector ret;
-  string path = JoinPathSegments(GetTestExecutableDirectory(), name);
+  RowSetVector rowsets;
+  const string path = JoinPathSegments(GetTestExecutableDirectory(), name);
   faststring data;
   CHECK_OK_PREPEND(ReadFileToString(Env::Default(), path, &data),
-                   strings::Substitute("Unable to load test data file $0", path));
-  vector<string> lines = strings::Split(data.ToString(), "\n");
+                   strings::Substitute("unable to load test data file $0", path));
+  const vector<string> lines = strings::Split(data.ToString(), "\n");
   for (const auto& line : lines) {
     if (line.empty() || line[0] == '#') continue;
-    vector<string> fields = strings::Split(line, "\t");
+    const vector<string> fields = strings::Split(line, "\t");
     CHECK_EQ(3, fields.size()) << "Expected 3 fields on line: " << line;
-    int size_mb = ParseLeadingInt32Value(fields[0], -1);
+    const int size_mb = ParseLeadingInt32Value(fields[0], -1);
     CHECK_GE(size_mb, 1) << "Expected size at least 1MB on line: " << line;
-    ret.emplace_back(new MockDiskRowSet(fields[1] /* min key */,
-                                        fields[2] /* max key */,
-                                        size_mb * 1024 * 1024));
+    rowsets.emplace_back(new MockDiskRowSet(fields[1] /* min key */,
+                                            fields[2] /* max key */,
+                                            size_mb * 1024 * 1024));
   }
 
-  return ret;
+  return rowsets;
 }
 
-// Realistic test using data scraped from a tablet containing 200+GB of YCSB data.
-// This test can be used as a benchmark for optimizing the compaction policy,
-// and also serves as a basic regression/stress test using real data.
+// Realistic test using data scraped from a tablet containing 200GB+ of YCSB
+// data. This test can be used as a benchmark for optimizing the compaction
+// policy, and also serves as a basic regression/stress test using real data.
 TEST_F(TestCompactionPolicy, TestYcsbCompaction) {
-  RowSetVector vec = LoadFile("ycsb-test-rowsets.tsv");
+  if (!AllowSlowTests()) {
+    LOG(WARNING) << "test is skipped; set KUDU_ALLOW_SLOW_TESTS=1 to run";
+    return;
+  }
+  const RowSetVector rowsets = LoadFile("ycsb-test-rowsets.tsv");
   RowSetTree tree;
-  ASSERT_OK(tree.Reset(vec));
+  ASSERT_OK(tree.Reset(rowsets));
   vector<double> qualities;
   for (int budget_mb : {128, 256, 512, 1024}) {
     BudgetedCompactionPolicy policy(budget_mb);
 
     unordered_set<RowSet*> picked;
-    double quality = 0;
-    LOG_TIMING(INFO, strings::Substitute("Computing compaction with $0MB budget", budget_mb)) {
-      ASSERT_OK(policy.PickRowSets(tree, &picked, &quality, nullptr));
+    double quality = 0.0;
+    LOG_TIMING(INFO, strings::Substitute("computing compaction with $0MB budget",
+                                         budget_mb)) {
+      ASSERT_OK(policy.PickRowSets(tree, &picked, &quality, /*log=*/nullptr));
     }
     LOG(INFO) << "quality=" << quality;
     int total_size = 0;
@@ -188,26 +252,28 @@ TEST_F(TestCompactionPolicy, TestYcsbCompaction) {
     qualities.push_back(quality);
   }
 
-  // Given increasing budgets, our solutions should also be higher quality.
-  ASSERT_TRUE(std::is_sorted(qualities.begin(), qualities.end()))
-      << qualities;
+  // Since the budget increased in each iteration, the qualities should be
+  // non-decreasing.
+  ASSERT_TRUE(std::is_sorted(qualities.begin(), qualities.end())) << qualities;
 }
 
 // Regression test for KUDU-2251 which ensures that large (> 2GiB) rowsets don't
 // cause integer overflow in compaction planning.
 TEST_F(TestCompactionPolicy, KUDU2251) {
-  RowSetVector vec = {
+  // Same arrangement as in TestBudgetedSelection.
+  const RowSetVector rowsets = {
     std::make_shared<MockDiskRowSet>("C", "c", 1L << 31),
     std::make_shared<MockDiskRowSet>("B", "a", 1L << 32),
     std::make_shared<MockDiskRowSet>("A", "b", 1L << 33)
   };
 
-  const int kBudgetMb = 1L << 30; // enough to select all
+  constexpr auto kBudgetMb = 1L << 30; // Enough to select all rowsets.
   unordered_set<RowSet*> picked;
-  double quality = 0;
-  NO_FATALS(RunTestCase(vec, kBudgetMb, &picked, &quality));
+  double quality = 0.0;
+  NO_FATALS(RunTestCase(rowsets, kBudgetMb, &picked, &quality));
   ASSERT_EQ(3, picked.size());
-  ASSERT_GE(quality, 1.0);
+  ASSERT_GT(quality, 1.0);
+  ASSERT_LT(quality, 2.0);
 }
 
 } // namespace tablet