kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From aw...@apache.org
Subject [kudu] branch branch-1.9.x updated: KUDU-2701 Fix compaction loop due to using wrong rowset size
Date Wed, 20 Feb 2019 05:31:28 GMT
This is an automated email from the ASF dual-hosted git repository.

awong pushed a commit to branch branch-1.9.x
in repository https://gitbox.apache.org/repos/asf/kudu.git


The following commit(s) were added to refs/heads/branch-1.9.x by this push:
     new 949609b  KUDU-2701 Fix compaction loop due to using wrong rowset size
949609b is described below

commit 949609bdd01792100f7f343a26f85a1250e32ff0
Author: Will Berkeley <wdberkeley@gmail.org>
AuthorDate: Thu Feb 14 14:00:06 2019 -0800

    KUDU-2701 Fix compaction loop due to using wrong rowset size
    
    Compaction policy originally evaluated the size of a rowset to be the
    size of the rowset's base data and its REDOs. This size is used to
    calculate the probability mass of the rowset and the weight of the
    rowset in the compaction knapsack problem. Mistakenly, it was also used
    as the size of a rowset for KUDU-1400 small rowset compaction policy.
    This is wrong- the size of the whole rowset should be used. The reason
    for this is the following: small rowset compaction compacts
    rowsets when the number of rowsets is reduced. The number of rowsets
    produced depends on the total size of the data when written. If a
    partial size of the rowset is used, this can lead to bad decisions being
    made about compaction. For example, I discovered a case where a tablet
    had 8 rowsets of "size" 16MB. In reality, this size was the base data
    size plus REDOs. Small rowset compaction policy determined that it would
    be good to compact these 8 rowsets, believing that it would produce 4
    rowsets of size 32MB. However, the rowsets were actually 32MB in size,
    and compacting them produced 8 rowsets of size 32MB identical to the
    previous 8, and therefore 8 rowsets that appeared to be of size 16MB to
    compaction policy. Thus these 8 were chosen to be compacted, and so
    on...
    
    This patch changes the small rowset compaction policy code to use the
    full size of the rowsets. All other uses of size remain the same. It's
    not totally clear to me why just base data and REDOs are used, but in
    any case changing it would change CDFs and change knapsack calculations,
    which could lead to pretty big differences in compaction behavior.
    Since, outside this bug, compaction is working fine, and since this
    patch is targeted to unblock 1.9, I opted to make a minimal change to
    fix the bug and not evaluate any larger chance to compaction policy.
    
    I tested this code on the cluster where I originally encountered the
    compaction loop. The loop did not reoccur and the updated compaction
    policy successfully compacted small rowsets together for several hours
    without becoming stuck. It finally converged to a stable end state. I
    also ran a number of sequential and random insert workloads on this
    cluster and saw no problems with compactions looping or getting stuck,
    even under high load or when insertions ended and the tablet was free to
    catch up on compactions.
    
    Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007
    Reviewed-on: http://gerrit.cloudera.org:8080/12488
    Reviewed-by: Andrew Wong <awong@cloudera.com>
    Tested-by: Andrew Wong <awong@cloudera.com>
    Reviewed-by: Alexey Serbin <aserbin@cloudera.com>
    Reviewed-on: http://gerrit.cloudera.org:8080/12532
    Tested-by: Kudu Jenkins
---
 src/kudu/tablet/compaction_policy.cc |  8 ++++----
 src/kudu/tablet/rowset_info.cc       | 22 +++++++++++++---------
 src/kudu/tablet/rowset_info.h        | 15 +++++++++++----
 3 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/src/kudu/tablet/compaction_policy.cc b/src/kudu/tablet/compaction_policy.cc
index 77af79b..e740ac4 100644
--- a/src/kudu/tablet/compaction_policy.cc
+++ b/src/kudu/tablet/compaction_policy.cc
@@ -124,7 +124,7 @@ struct KnapsackTraits {
   typedef const RowSetInfo* item_type;
   typedef double value_type;
   static int get_weight(item_type item) {
-    return item->size_mb();
+    return item->base_and_redos_size_mb();
   }
   static value_type get_value(item_type item) {
     return item->value();
@@ -179,11 +179,11 @@ class BoundCalculator {
     std::push_heap(fractional_solution_.begin(),
                    fractional_solution_.end(),
                    compareByDescendingDensity);
-    current_weight_ += candidate.size_mb();
+    current_weight_ += candidate.base_and_redos_size_mb();
     current_value_ += candidate.value();
     const RowSetInfo* top = fractional_solution_.front();
-    while (current_weight_ - top->size_mb() > max_weight_) {
-      current_weight_ -= top->size_mb();
+    while (current_weight_ - top->base_and_redos_size_mb() > max_weight_) {
+      current_weight_ -= top->base_and_redos_size_mb();
       current_value_ -= top->value();
       std::pop_heap(fractional_solution_.begin(),
                     fractional_solution_.end(),
diff --git a/src/kudu/tablet/rowset_info.cc b/src/kudu/tablet/rowset_info.cc
index 7adb953..5089a1d 100644
--- a/src/kudu/tablet/rowset_info.cc
+++ b/src/kudu/tablet/rowset_info.cc
@@ -191,7 +191,7 @@ double WidthByDataSize(const Slice& prev, const Slice& next,
 
   for (const auto& rs_rsi : active) {
     double fraction = StringFractionInRange(rs_rsi.second, prev, next);
-    weight += rs_rsi.second->size_bytes() * fraction;
+    weight += rs_rsi.second->base_and_redos_size_bytes() * fraction;
   }
 
   return weight;
@@ -449,9 +449,12 @@ RowSetInfo::RowSetInfo(RowSet* rs, double init_cdf)
       cdf_max_key_(init_cdf),
       extra_(new ExtraData()) {
   extra_->rowset = rs;
-  extra_->size_bytes = rs->OnDiskBaseDataSizeWithRedos();
+  extra_->base_and_redos_size_bytes = rs->OnDiskBaseDataSizeWithRedos();
+  extra_->size_bytes = rs->OnDiskSize();
   extra_->has_bounds = rs->GetBounds(&extra_->min_key, &extra_->max_key).ok();
-  size_mb_ = std::max(implicit_cast<int>(extra_->size_bytes / 1024 / 1024), kMinSizeMb);
+  base_and_redos_size_mb_ =
+      std::max(implicit_cast<int>(extra_->base_and_redos_size_bytes / 1024 / 1024),
+                                  kMinSizeMb);
 }
 
 uint64_t RowSetInfo::size_bytes(const ColumnId& col_id) const {
@@ -461,21 +464,22 @@ uint64_t RowSetInfo::size_bytes(const ColumnId& col_id) const {
 void RowSetInfo::FinalizeCDFVector(double quot, vector<RowSetInfo>* vec) {
   if (quot == 0) return;
   for (RowSetInfo& cdf_rs : *vec) {
-    CHECK_GT(cdf_rs.size_mb_, 0) << "Expected file size to be at least 1MB "
-                                 << "for RowSet " << cdf_rs.rowset()->ToString()
-                                 << ", was " << cdf_rs.size_bytes()
-                                 << " bytes.";
+    CHECK_GT(cdf_rs.base_and_redos_size_mb_, 0)
+        << "Expected file size to be at least 1MB "
+        << "for RowSet " << cdf_rs.rowset()->ToString()
+        << ", was " << cdf_rs.base_and_redos_size_bytes()
+        << " bytes.";
     cdf_rs.cdf_min_key_ /= quot;
     cdf_rs.cdf_max_key_ /= quot;
     cdf_rs.value_ = ComputeRowsetValue(cdf_rs.width(), cdf_rs.extra_->size_bytes);
-    cdf_rs.density_ = cdf_rs.value_ / cdf_rs.size_mb_;
+    cdf_rs.density_ = cdf_rs.value_ / cdf_rs.base_and_redos_size_mb_;
   }
 }
 
 string RowSetInfo::ToString() const {
   string ret;
   ret.append(rowset()->ToString());
-  StringAppendF(&ret, "(% 3dM) [%.04f, %.04f]", size_mb_,
+  StringAppendF(&ret, "(% 3dM) [%.04f, %.04f]", base_and_redos_size_mb_,
                 cdf_min_key_, cdf_max_key_);
   if (extra_->has_bounds) {
     ret.append(" [").append(KUDU_REDACT(Slice(extra_->min_key).ToDebugString()));
diff --git a/src/kudu/tablet/rowset_info.h b/src/kudu/tablet/rowset_info.h
index a8290d3..af2d07d 100644
--- a/src/kudu/tablet/rowset_info.h
+++ b/src/kudu/tablet/rowset_info.h
@@ -69,8 +69,11 @@ class RowSetInfo {
                             std::vector<KeyRange>* ranges);
 
   uint64_t size_bytes(const ColumnId& col_id) const;
+  uint64_t base_and_redos_size_bytes() const {
+    return extra_->base_and_redos_size_bytes;
+  }
   uint64_t size_bytes() const { return extra_->size_bytes; }
-  int size_mb() const { return size_mb_; }
+  int base_and_redos_size_mb() const { return base_and_redos_size_mb_; }
 
   // Return the value of the CDF at the minimum key of this candidate.
   double cdf_min_key() const { return cdf_min_key_; }
@@ -113,9 +116,10 @@ class RowSetInfo {
 
   static void FinalizeCDFVector(double quot, std::vector<RowSetInfo>* vec);
 
-  // The size in MB, already clamped so that all rowsets have size at least
-  // 1MB. This is cached to avoid the branch during the selection hot path.
-  int size_mb_;
+  // The size of the base data and redos in MB, already clamped so that all
+  // rowsets have size at least 1MB. This is cached to avoid the branch during
+  // the selection hot path.
+  int base_and_redos_size_mb_;
 
   double cdf_min_key_, cdf_max_key_;
 
@@ -139,6 +143,9 @@ class RowSetInfo {
   // These are ref-counted so that RowSetInfo is copyable.
   struct ExtraData : public RefCounted<ExtraData> {
     // Cached version of rowset_->OnDiskBaseDataSizeWithRedos().
+    uint64_t base_and_redos_size_bytes;
+
+    // Cached version of rowset_->OnDiskSize().
     uint64_t size_bytes;
 
     // True if the RowSet has known bounds.


Mime
View raw message