[kudu] 01/02: KUDU-2929: don't do nothing when under memory pressure

[al...@apache.org]

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

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

commit bc4850f271236ef8722379ed1727568aff198fd9
Author: Andrew Wong <aw...@apache.org>
AuthorDate: Wed Nov 6 20:16:05 2019 -0800

    KUDU-2929: don't do nothing when under memory pressure
    
    When we're under memory pressure, we may exit early from considering
    ops if there are none that we know will free memory. This might be
    because we don't want to spend more memory performing an op when we are
    already under memory pressure if we don't know that op is anchoring
    memory (obvious in the case of MRS or DMS flushes, less so in the case
    of compactions).
    
    While nice in theory, I don't think this behavior was desirable. Yes, we
    want to prioritize ops that definitely free memory over those that might
    not, but doing _something_ seems better than nothing.
    
    So rather than returning early if there aren't ops that free memory,
    let's continue walking through valid ops (in order of what frees the
    most WALs, then what frees the most on-disk data, then what has the best
    perf improvement).
    
    Change-Id: I030f9ef379af501fe7bd2f42906ec2f9ea16dbde
    Reviewed-on: http://gerrit.cloudera.org:8080/14650
    Reviewed-by: Adar Dembo <ad...@cloudera.com>
    Tested-by: Andrew Wong <aw...@cloudera.com>
---
 src/kudu/tserver/tablet_server-test.cc    | 45 +++++++++++++++++++++++++++++++
 src/kudu/util/maintenance_manager-test.cc | 26 +++++++++++++++++-
 src/kudu/util/maintenance_manager.cc      | 11 ++------
 3 files changed, 72 insertions(+), 10 deletions(-)

diff --git a/src/kudu/tserver/tablet_server-test.cc b/src/kudu/tserver/tablet_server-test.cc
index cd5dcf8..9abf3d8 100644
--- a/src/kudu/tserver/tablet_server-test.cc
+++ b/src/kudu/tserver/tablet_server-test.cc
@@ -158,7 +158,9 @@ DEFINE_int32(delete_tablet_bench_num_flushes, 200,
              "Number of disk row sets to flush in the delete tablet benchmark");
 
 DECLARE_bool(crash_on_eio);
+DECLARE_bool(enable_flush_memrowset);
 DECLARE_bool(enable_maintenance_manager);
+DECLARE_bool(enable_rowset_compaction);
 DECLARE_bool(fail_dns_resolution);
 DECLARE_bool(rowset_metadata_store_keys);
 DECLARE_double(cfile_inject_corruption);
@@ -169,6 +171,7 @@ DECLARE_int32(flush_threshold_secs);
 DECLARE_int32(fs_data_dirs_available_space_cache_seconds);
 DECLARE_int32(fs_target_data_dirs_per_tablet);
 DECLARE_int32(maintenance_manager_num_threads);
+DECLARE_int32(memory_pressure_percentage);
 DECLARE_int32(metrics_retirement_age_ms);
 DECLARE_int32(scanner_batch_size_rows);
 DECLARE_int32(scanner_gc_check_interval_us);
@@ -859,6 +862,48 @@ TEST_F(TabletServerTest, TestEIODuringDelete) {
   ASSERT_OK(admin_proxy_->DeleteTablet(req, &resp, &rpc));
 }
 
+class TabletServerMaintenanceMemoryPressureTest : public TabletServerTestBase {
+ public:
+  void SetUp() override {
+    NO_FATALS(TabletServerTestBase::SetUp());
+    FLAGS_enable_maintenance_manager = true;
+    FLAGS_flush_threshold_secs = 1;
+    FLAGS_memory_pressure_percentage = 0;
+
+    // While setting up rowsets, disable compactions and flushing. Do this
+    // before doing anything so we can have tighter control over the flushing
+    // of our rowsets.
+    FLAGS_enable_rowset_compaction = false;
+    FLAGS_enable_flush_memrowset = false;
+    NO_FATALS(StartTabletServer(/*num_data_dirs=*/1));
+  }
+};
+
+// Regression test for KUDU-2929. Previously, when under memory pressure, we
+// would never compact, even if there were nothing else to do. We'll simulate
+// this by flushing some overlapping rowsets and then making sure we compact.
+TEST_F(TabletServerMaintenanceMemoryPressureTest, TestCompactWhileUnderMemoryPressure) {
+  // Insert sets of overlapping rows.
+  // Since we're under memory pressure, we'll flush as soon as we're able.
+  NO_FATALS(InsertTestRowsDirect(1, 1));
+  NO_FATALS(InsertTestRowsDirect(3, 1));
+  FLAGS_enable_flush_memrowset = true;
+  ASSERT_EVENTUALLY([&] {
+    ASSERT_EQ(1, tablet_replica_->tablet()->num_rowsets());
+  });
+  NO_FATALS(InsertTestRowsDirect(2, 1));
+  ASSERT_EVENTUALLY([&] {
+    ASSERT_EQ(2, tablet_replica_->tablet()->num_rowsets());
+  });
+
+  // Even though we're under memory pressure, we should see compactions because
+  // there's nothing else to do.
+  FLAGS_enable_rowset_compaction = true;
+  ASSERT_EVENTUALLY([&] {
+    ASSERT_EQ(1, tablet_replica_->tablet()->num_rowsets());
+  });
+}
+
 TEST_F(TabletServerTest, TestInsert) {
   WriteRequestPB req;
 
diff --git a/src/kudu/util/maintenance_manager-test.cc b/src/kudu/util/maintenance_manager-test.cc
index d75f9a1..51f9fcd 100644
--- a/src/kudu/util/maintenance_manager-test.cc
+++ b/src/kudu/util/maintenance_manager-test.cc
@@ -267,7 +267,7 @@ TEST_F(MaintenanceManagerTest, TestNewOpsDontGetScheduledDuringUnregister) {
 
 // Test that we'll run an operation that doesn't improve performance when memory
 // pressure gets high.
-TEST_F(MaintenanceManagerTest, TestMemoryPressure) {
+TEST_F(MaintenanceManagerTest, TestMemoryPressurePrioritizesMemory) {
   TestMaintenanceOp op("op", MaintenanceOp::HIGH_IO_USAGE);
   op.set_ram_anchored(100);
   manager_->RegisterOp(&op);
@@ -285,6 +285,30 @@ TEST_F(MaintenanceManagerTest, TestMemoryPressure) {
   manager_->UnregisterOp(&op);
 }
 
+// Test that when under memory pressure, we'll run an op that doesn't improve
+// memory pressure if there's nothing else to do.
+TEST_F(MaintenanceManagerTest, TestMemoryPressurePerformsNoMemoryOp) {
+  TestMaintenanceOp op("op", MaintenanceOp::HIGH_IO_USAGE);
+  op.set_ram_anchored(0);
+  manager_->RegisterOp(&op);
+
+  // At first, we don't want to run this, since there is no perf_improvement.
+  SleepFor(MonoDelta::FromMilliseconds(20));
+  ASSERT_EQ(0, op.DurationHistogram()->TotalCount());
+
+  // Now fake that the server is under memory pressure and make our op runnable
+  // by giving it a perf score.
+  indicate_memory_pressure_ = true;
+  op.set_perf_improvement(1);
+
+  // Even though we're under memory pressure, and even though our op doesn't
+  // have any ram anchored, there's nothing else to do, so we run our op.
+  ASSERT_EVENTUALLY([&] {
+    ASSERT_EQ(op.DurationHistogram()->TotalCount(), 1);
+  });
+  manager_->UnregisterOp(&op);
+}
+
 // Test that ops are prioritized correctly when we add log retention.
 TEST_F(MaintenanceManagerTest, TestLogRetentionPrioritization) {
   const int64_t kMB = 1024 * 1024;
diff --git a/src/kudu/util/maintenance_manager.cc b/src/kudu/util/maintenance_manager.cc
index b8b3a29..6e0b964 100644
--- a/src/kudu/util/maintenance_manager.cc
+++ b/src/kudu/util/maintenance_manager.cc
@@ -435,16 +435,9 @@ pair<MaintenanceOp*, string> MaintenanceManager::FindBestOp() {
   }
 
   // Look at free memory. If it is dangerously low, we must select something
-  // that frees memory-- the op with the most anchored memory.
+  // that frees memory -- the op with the most anchored memory.
   double capacity_pct;
-  if (memory_pressure_func_(&capacity_pct)) {
-    if (!most_ram_anchored_op) {
-      string msg = StringPrintf("System under memory pressure "
-          "(%.2f%% of limit used). However, there are no ops currently "
-          "runnable which would free memory.", capacity_pct);
-      KLOG_EVERY_N_SECS(WARNING, 5) << msg;
-      return {nullptr, msg};
-    }
+  if (memory_pressure_func_(&capacity_pct) && most_ram_anchored_op) {
     string note = StringPrintf("under memory pressure (%.2f%% used, "
                                "can flush %" PRIu64 " bytes)",
                                capacity_pct, most_ram_anchored);