kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From a...@apache.org
Subject kudu git commit: KUDU-2194: use a common trace metric prefix to avoid overflowing intern map
Date Thu, 19 Oct 2017 22:32:14 GMT
Repository: kudu
Updated Branches:
  refs/heads/master a6f2322b2 -> f2eb6c277


KUDU-2194: use a common trace metric prefix to avoid overflowing intern map

Without a prefix, we end up prefixing trace metric names with the
corresponding thread pool's name, and each data directory has a uniquely
named thread pool. The magic number of data directories needed to overflow
the intern map is 34, though it'll be lower in production as the server will
have created other thread pools.

Change-Id: I2d95e440c3fef76d7cbfe568a014bab9cda7f329
Reviewed-on: http://gerrit.cloudera.org:8080/8340
Reviewed-by: Todd Lipcon <todd@apache.org>
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <awong@cloudera.com>


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

Branch: refs/heads/master
Commit: f2eb6c277860890b1177071ae265bace5837a0f1
Parents: a6f2322
Author: Adar Dembo <adar@cloudera.com>
Authored: Thu Oct 19 14:48:19 2017 -0700
Committer: Adar Dembo <adar@cloudera.com>
Committed: Thu Oct 19 22:31:24 2017 +0000

----------------------------------------------------------------------
 src/kudu/fs/data_dirs-test.cc | 24 +++++++++++++++++++++---
 src/kudu/fs/data_dirs.cc      |  1 +
 src/kudu/util/threadpool.cc   |  8 ++++----
 3 files changed, 26 insertions(+), 7 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/f2eb6c27/src/kudu/fs/data_dirs-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/fs/data_dirs-test.cc b/src/kudu/fs/data_dirs-test.cc
index 0c29046..6f4c589 100644
--- a/src/kudu/fs/data_dirs-test.cc
+++ b/src/kudu/fs/data_dirs-test.cc
@@ -364,10 +364,9 @@ TEST_F(DataDirsTest, TestLoadBalancingBias) {
 
 class DataDirManagerTest : public DataDirsTest {
  public:
-  DataDirManagerTest() :
-    test_roots_(JoinPathSegmentsV(GetDirNames(kNumDirs), "root")) {}
-
   void SetUp() override {
+    test_roots_ = JoinPathSegmentsV(GetDirNames(GetNumDirs()), "root");
+
     // Don't call DataDirsTest::SetUp() to avoid creating the default directory
     // manager.
     KuduTest::SetUp();
@@ -378,6 +377,8 @@ class DataDirManagerTest : public DataDirsTest {
         DataDirManagerOptions(), &dd_manager_);
   }
 
+  virtual int GetNumDirs() const { return kNumDirs; }
+
  protected:
   // The test roots. Data will be placed in a data directory within the root.
   // E.g. Test root: /test_data_dir_0/root, Data dir: /test_data_dir_0/root/data
@@ -425,5 +426,22 @@ TEST_F(DataDirManagerTest, TestOpenWithFailedDirs) {
   FLAGS_env_inject_eio = 0;
 }
 
+class TooManyDataDirManagerTest : public DataDirManagerTest {
+ public:
+  // TraceMetrics::g_intern_map has a limited number of entries, and each data
+  // dir used to consume three of them via its threadpool. This value was just
+  // enough to exceed the map's capacity.
+  int GetNumDirs() const override { return 34; }
+};
+
+// Regression test for KUDU-2194.
+TEST_F(TooManyDataDirManagerTest, TestTooManyInternedStrings) {
+  for (const auto& r : test_roots_) {
+    ASSERT_OK(env_->CreateDir(r));
+  }
+  ASSERT_OK(DataDirManager::CreateNewForTests(
+      env_, test_roots_, DataDirManagerOptions(), &dd_manager_));
+}
+
 } // namespace fs
 } //namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/f2eb6c27/src/kudu/fs/data_dirs.cc
----------------------------------------------------------------------
diff --git a/src/kudu/fs/data_dirs.cc b/src/kudu/fs/data_dirs.cc
index 635a4a8..5e9b0d0 100644
--- a/src/kudu/fs/data_dirs.cc
+++ b/src/kudu/fs/data_dirs.cc
@@ -483,6 +483,7 @@ Status DataDirManager::Open() {
     gscoped_ptr<ThreadPool> pool;
     RETURN_NOT_OK(ThreadPoolBuilder(Substitute("data dir $0", i))
                   .set_max_threads(1)
+                  .set_trace_metric_prefix("data dirs")
                   .Build(&pool));
 
     // Figure out what filesystem the data directory is on.

http://git-wip-us.apache.org/repos/asf/kudu/blob/f2eb6c27/src/kudu/util/threadpool.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/threadpool.cc b/src/kudu/util/threadpool.cc
index d36d455..59a0650 100644
--- a/src/kudu/util/threadpool.cc
+++ b/src/kudu/util/threadpool.cc
@@ -43,6 +43,7 @@ namespace kudu {
 
 using std::deque;
 using std::shared_ptr;
+using std::string;
 using std::unique_ptr;
 using strings::Substitute;
 
@@ -82,15 +83,14 @@ class ClosureRunnable : public Runnable {
 // ThreadPoolBuilder
 ////////////////////////////////////////////////////////
 
-ThreadPoolBuilder::ThreadPoolBuilder(std::string name)
+ThreadPoolBuilder::ThreadPoolBuilder(string name)
     : name_(std::move(name)),
       min_threads_(0),
       max_threads_(base::NumCPUs()),
       max_queue_size_(std::numeric_limits<int>::max()),
       idle_timeout_(MonoDelta::FromMilliseconds(500)) {}
 
-ThreadPoolBuilder& ThreadPoolBuilder::set_trace_metric_prefix(
-    const std::string& prefix) {
+ThreadPoolBuilder& ThreadPoolBuilder::set_trace_metric_prefix(const string& prefix)
{
   trace_metric_prefix_ = prefix;
   return *this;
 }
@@ -322,7 +322,7 @@ ThreadPool::ThreadPool(const ThreadPoolBuilder& builder)
     total_queued_tasks_(0),
     tokenless_(NewToken(ExecutionMode::CONCURRENT)),
     metrics_(builder.metrics_) {
-  std::string prefix = !builder.trace_metric_prefix_.empty() ?
+  string prefix = !builder.trace_metric_prefix_.empty() ?
       builder.trace_metric_prefix_ : builder.name_;
 
   queue_time_trace_metric_name_ = TraceMetrics::InternName(


Mime
View raw message