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

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

commit 10d1be051bd56c4f74a2193c6962aa7fcaf92290
Author: Tamas Mate <tm...@cloudera.com>
AuthorDate: Tue Jun 18 10:03:10 2019 -0700

    IMPALA-7734: Catalog and Statestore memz page shows useless memory
    
    The MemTracker is removed from StatestoreD main and CatalogD main as
    it is not used. The /memz page related metrics are refactored to a
    a separate file. When the MemTracker is not available during callback
    initialization it should not be part of the callback either. Therefore,
    when MemTracker is not part of the callback it is not displayed.
    
    Before/after pictures of the /memz page can be found in the Jira.
    
    Change-Id: I1a42434b98aec1d9e0be6ae52325049378fdde23
    Reviewed-on: http://gerrit.cloudera.org:8080/13670
    Reviewed-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
    Tested-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
---
 be/src/catalog/catalogd-main.cc                    |   4 +-
 be/src/runtime/exec-env.cc                         |   2 +-
 be/src/statestore/statestored-main.cc              |   4 +-
 be/src/util/CMakeLists.txt                         |   1 +
 be/src/util/default-path-handlers.cc               |  85 +------------
 be/src/util/default-path-handlers.h                |   4 +-
 be/src/util/memusage-path-handlers.cc              | 140 +++++++++++++++++++++
 ...lt-path-handlers.h => memusage-path-handlers.h} |  19 ++-
 www/memz.tmpl                                      |   4 +
 9 files changed, 162 insertions(+), 101 deletions(-)

diff --git a/be/src/catalog/catalogd-main.cc b/be/src/catalog/catalogd-main.cc
index 21fce3c..67d6f39 100644
--- a/be/src/catalog/catalogd-main.cc
+++ b/be/src/catalog/catalogd-main.cc
@@ -26,7 +26,6 @@
 #include "rpc/authentication.h"
 #include "rpc/rpc-trace.h"
 #include "rpc/thrift-server.h"
-#include "runtime/mem-tracker.h"
 #include "service/fe-support.h"
 #include "util/common-metrics.h"
 #include "util/debug-util.h"
@@ -63,12 +62,11 @@ int CatalogdMain(int argc, char** argv) {
   InitCommonRuntime(argc, argv, true);
   InitFeSupport();
 
-  MemTracker process_mem_tracker;
   scoped_ptr<Webserver> webserver(new Webserver());
   scoped_ptr<MetricGroup> metrics(new MetricGroup("catalog"));
 
   if (FLAGS_enable_webserver) {
-    AddDefaultUrlCallbacks(webserver.get(), &process_mem_tracker, 
metrics.get());
+    AddDefaultUrlCallbacks(webserver.get(), metrics.get());
     ABORT_IF_ERROR(webserver->Start());
   } else {
     LOG(INFO) << "Not starting webserver";
diff --git a/be/src/runtime/exec-env.cc b/be/src/runtime/exec-env.cc
index 254d3ef..b4b4abd 100644
--- a/be/src/runtime/exec-env.cc
+++ b/be/src/runtime/exec-env.cc
@@ -334,7 +334,7 @@ Status ExecEnv::Init() {
 
   // Start services in order to ensure that dependencies between them are met
   if (enable_webserver_) {
-    AddDefaultUrlCallbacks(webserver_.get(), mem_tracker_.get(), 
metrics_.get());
+    AddDefaultUrlCallbacks(webserver_.get(), metrics_.get(), 
mem_tracker_.get());
     RETURN_IF_ERROR(webserver_->Start());
   } else {
     LOG(INFO) << "Not starting webserver";
diff --git a/be/src/statestore/statestored-main.cc 
b/be/src/statestore/statestored-main.cc
index d7db794..03f6869 100644
--- a/be/src/statestore/statestored-main.cc
+++ b/be/src/statestore/statestored-main.cc
@@ -26,7 +26,6 @@
 #include "common/logging.h"
 #include "common/status.h"
 #include "rpc/rpc-trace.h"
-#include "runtime/mem-tracker.h"
 #include "statestore/statestore.h"
 #include "util/common-metrics.h"
 #include "util/debug-util.h"
@@ -50,12 +49,11 @@ int StatestoredMain(int argc, char** argv) {
   FLAGS_webserver_port = 25010;
   InitCommonRuntime(argc, argv, false);
 
-  MemTracker mem_tracker;
   scoped_ptr<Webserver> webserver(new Webserver());
   scoped_ptr<MetricGroup> metrics(new MetricGroup("statestore"));
 
   if (FLAGS_enable_webserver) {
-    AddDefaultUrlCallbacks(webserver.get(), &mem_tracker, metrics.get());
+    AddDefaultUrlCallbacks(webserver.get(), metrics.get());
     ABORT_IF_ERROR(webserver->Start());
   } else {
     LOG(INFO) << "Not starting webserver";
diff --git a/be/src/util/CMakeLists.txt b/be/src/util/CMakeLists.txt
index e07ee59..b12ae2d 100644
--- a/be/src/util/CMakeLists.txt
+++ b/be/src/util/CMakeLists.txt
@@ -61,6 +61,7 @@ add_library(Util
   logging-support.cc
   mem-info.cc
   memory-metrics.cc
+  memusage-path-handlers.cc
   metrics.cc
   min-max-filter.cc
   min-max-filter-ir.cc
diff --git a/be/src/util/default-path-handlers.cc 
b/be/src/util/default-path-handlers.cc
index 0d5f493..21bc1c6 100644
--- a/be/src/util/default-path-handlers.cc
+++ b/be/src/util/default-path-handlers.cc
@@ -22,7 +22,6 @@
 #include <sys/stat.h>
 #include <boost/algorithm/string.hpp>
 #include <boost/bind.hpp>
-#include <gperftools/malloc_extension.h>
 #include <gutil/strings/substitute.h>
 
 #include "common/logging.h"
@@ -37,8 +36,8 @@
 #include "util/disk-info.h"
 #include "util/jni-util.h"
 #include "util/mem-info.h"
+#include "util/memusage-path-handlers.h"
 #include "util/pprof-path-handlers.h"
-#include "util/pretty-printer.h"
 #include "util/process-state-info.h"
 
 #include "common/names.h"
@@ -128,76 +127,6 @@ void FlagsHandler(const Webserver::WebRequest& req, 
Document* document) {
   document->AddMember("flags", flag_arr, document->GetAllocator());
 }
 
-// Registered to handle "/memz"
-void MemUsageHandler(MemTracker* mem_tracker, MetricGroup* metric_group,
-    const Webserver::WebRequest& req, Document* document) {
-  DCHECK(mem_tracker != NULL);
-  Value mem_limit(PrettyPrinter::Print(mem_tracker->limit(), 
TUnit::BYTES).c_str(),
-      document->GetAllocator());
-  document->AddMember("mem_limit", mem_limit, document->GetAllocator());
-  Value consumption(
-      PrettyPrinter::Print(mem_tracker->consumption(), TUnit::BYTES).c_str(),
-      document->GetAllocator());
-  document->AddMember("consumption", consumption, document->GetAllocator());
-
-  stringstream ss;
-#if defined(ADDRESS_SANITIZER) || defined(THREAD_SANITIZER)
-  ss << "Memory tracking is not available with address or thread sanitizer 
builds.";
-#else
-  char buf[2048];
-  MallocExtension::instance()->GetStats(buf, 2048);
-  ss << string(buf);
-#endif
-
-  Value overview(ss.str().c_str(), document->GetAllocator());
-  document->AddMember("overview", overview, document->GetAllocator());
-
-  // Dump all mem trackers.
-  Value detailed(mem_tracker->LogUsage(MemTracker::UNLIMITED_DEPTH).c_str(),
-      document->GetAllocator());
-  document->AddMember("detailed", detailed, document->GetAllocator());
-
-  Value systeminfo(MemInfo::DebugString().c_str(), document->GetAllocator());
-  document->AddMember("systeminfo", systeminfo, document->GetAllocator());
-
-  if (metric_group != nullptr) {
-    MetricGroup* aggregate_group = metric_group->FindChildGroup("memory");
-    if (aggregate_group != nullptr) {
-      Value json_metrics(kObjectType);
-      aggregate_group->ToJson(false, document, &json_metrics);
-      document->AddMember(
-          "aggregate_metrics", json_metrics["metrics"], 
document->GetAllocator());
-    }
-    MetricGroup* jvm_group = metric_group->FindChildGroup("jvm");
-    if (jvm_group != nullptr) {
-      Value jvm(kObjectType);
-      jvm_group->ToJson(false, document, &jvm);
-      Value heap(kArrayType);
-      Value non_heap(kArrayType);
-      Value total(kArrayType);
-      for (SizeType i = 0; i < jvm["metrics"].Size(); ++i) {
-        if (strstr(jvm["metrics"][i]["name"].GetString(), "total") != nullptr) 
{
-          total.PushBack(jvm["metrics"][i], document->GetAllocator());
-        } else if (strstr(jvm["metrics"][i]["name"].GetString(), "non-heap") 
!= nullptr) {
-          non_heap.PushBack(jvm["metrics"][i], document->GetAllocator());
-        } else if (strstr(jvm["metrics"][i]["name"].GetString(), "heap") != 
nullptr) {
-          heap.PushBack(jvm["metrics"][i], document->GetAllocator());
-        }
-      }
-      document->AddMember("jvm_total", total, document->GetAllocator());
-      document->AddMember("jvm_heap", heap, document->GetAllocator());
-      document->AddMember("jvm_non_heap", non_heap, document->GetAllocator());
-    }
-    MetricGroup* buffer_pool_group = 
metric_group->FindChildGroup("buffer-pool");
-    if (buffer_pool_group != nullptr) {
-      Value json_metrics(kObjectType);
-      buffer_pool_group->ToJson(false, document, &json_metrics);
-      document->AddMember(
-          "buffer_pool", json_metrics["metrics"], document->GetAllocator());
-    }
-  }
-}
-
 void JmxHandler(const Webserver::WebRequest& req, Document* document) {
   document->AddMember(rapidjson::StringRef(Webserver::ENABLE_PLAIN_JSON_KEY), 
true,
       document->GetAllocator());
@@ -301,8 +230,8 @@ void RootHandler(const Webserver::WebRequest& req, 
Document* document) {
   }
 }
 
-void AddDefaultUrlCallbacks(Webserver* webserver, MemTracker* 
process_mem_tracker,
-    MetricGroup* metric_group) {
+void AddDefaultUrlCallbacks(Webserver* webserver, MetricGroup* metric_group,
+    MemTracker* process_mem_tracker) {
   webserver->RegisterUrlCallback("/logs", "logs.tmpl", LogsHandler, true);
   webserver->RegisterUrlCallback("/varz", "flags.tmpl", FlagsHandler, true);
   if (JniUtil::is_jvm_inited()) {
@@ -311,13 +240,7 @@ void AddDefaultUrlCallbacks(Webserver* webserver, 
MemTracker* process_mem_tracke
     // (TODO): Switch to RawUrlCallback when it supports JSON content-type.
     webserver->RegisterUrlCallback("/jmx", "raw_text.tmpl", JmxHandler, true);
   }
-  if (process_mem_tracker != NULL) {
-    auto callback = [process_mem_tracker, metric_group]
-        (const Webserver::WebRequest& req, Document* doc) {
-      MemUsageHandler(process_mem_tracker, metric_group, req, doc);
-    };
-    webserver->RegisterUrlCallback("/memz", "memz.tmpl", callback, true);
-  }
+  AddMemUsageCallbacks(webserver, process_mem_tracker, metric_group);
 
 #if !defined(ADDRESS_SANITIZER) && !defined(THREAD_SANITIZER)
   // Remote (on-demand) profiling is disabled if the process is already being 
profiled.
diff --git a/be/src/util/default-path-handlers.h 
b/be/src/util/default-path-handlers.h
index 081179b..2483319 100644
--- a/be/src/util/default-path-handlers.h
+++ b/be/src/util/default-path-handlers.h
@@ -31,8 +31,8 @@ class MetricGroup;
 
 /// Adds a set of default path handlers to the webserver to display
 /// logs and configuration flags
-void AddDefaultUrlCallbacks(Webserver* webserver, MemTracker* 
process_mem_tracker = NULL,
-    MetricGroup* metric_group = NULL);
+void AddDefaultUrlCallbacks(Webserver* webserver, MetricGroup* metric_group = 
NULL,
+    MemTracker* process_mem_tracker = NULL);
 
 /// Registered to handle "/"
 /// Populates document with various system-wide information.
diff --git a/be/src/util/memusage-path-handlers.cc 
b/be/src/util/memusage-path-handlers.cc
new file mode 100644
index 0000000..d45fded
--- /dev/null
+++ b/be/src/util/memusage-path-handlers.cc
@@ -0,0 +1,140 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "util/memusage-path-handlers.h"
+
+#include <gperftools/malloc_extension.h>
+
+#include "runtime/mem-tracker.h"
+#include "util/common-metrics.h"
+#include "util/mem-info.h"
+#include "util/pretty-printer.h"
+
+#include "common/names.h"
+
+using namespace impala;
+using namespace rapidjson;
+
+/// A MemTracker tracks memory consumption and limits which will be displayed. 
This method
+/// adds the mem_limit and the current memory consumption to the rapidjson 
document. Also,
+/// it dumps all the additional mem trackers to the rapidjson document.
+void AddMemTracker(MemTracker* mem_tracker,
+    const Webserver::WebRequest& req, Document* document) {
+  DCHECK(mem_tracker != NULL);
+  Value mem_limit(PrettyPrinter::Print(mem_tracker->limit(), 
TUnit::BYTES).c_str(),
+      document->GetAllocator());
+  document->AddMember("mem_limit", mem_limit, document->GetAllocator());
+  Value consumption(
+      PrettyPrinter::Print(mem_tracker->consumption(), TUnit::BYTES).c_str(),
+      document->GetAllocator());
+  document->AddMember("consumption", consumption, document->GetAllocator());
+
+  // Dump all mem trackers.
+  Value detailed(mem_tracker->LogUsage(MemTracker::UNLIMITED_DEPTH).c_str(),
+      document->GetAllocator());
+  document->AddMember("detailed", detailed, document->GetAllocator());
+}
+
+/// Adds the following malloc data structures to the rapidjson document.
+/// - Total inuse memory by application.
+/// - Free memory(thread, central and page heap),
+/// - Freelist of central cache, each class.
+/// - Page heap freelist.
+void AddTCmallocOverview(const Webserver::WebRequest& req, Document* document) 
{
+  stringstream ss;
+#if defined(ADDRESS_SANITIZER) || defined(THREAD_SANITIZER)
+  ss << "Memory tracking is not available with address or thread sanitizer 
builds.";
+#else
+  char buf[2048];
+  MallocExtension::instance()->GetStats(buf, 2048);
+  ss << string(buf);
+#endif
+
+  Value overview(ss.str().c_str(), document->GetAllocator());
+  document->AddMember("overview", overview, document->GetAllocator());
+}
+
+/// Adds the system memory configuration to the rapidjson document.
+void AddSystemInfo(const Webserver::WebRequest& req, Document* document) {
+  Value systeminfo(MemInfo::DebugString().c_str(), document->GetAllocator());
+  document->AddMember("systeminfo", systeminfo, document->GetAllocator());
+}
+
+/// A MetricGroup is a container for a set of metric. This method extracts and 
adds the
+/// initialized JVM, buffer-pool and aggregated memory metrics to the 
rapidjson document
+/// from the given MetricGroup.
+void AddMetricGroup(MetricGroup* metric_group,
+    const Webserver::WebRequest& req, Document* document) {
+  if (metric_group != nullptr) {
+    MetricGroup* aggregate_group = metric_group->FindChildGroup("memory");
+    if (aggregate_group != nullptr) {
+      Value json_metrics(kObjectType);
+      aggregate_group->ToJson(false, document, &json_metrics);
+      document->AddMember(
+          "aggregate_metrics", json_metrics["metrics"], 
document->GetAllocator());
+    }
+    MetricGroup* jvm_group = metric_group->FindChildGroup("jvm");
+    if (jvm_group != nullptr) {
+      Value jvm(kObjectType);
+      jvm_group->ToJson(false, document, &jvm);
+      Value heap(kArrayType);
+      Value non_heap(kArrayType);
+      Value total(kArrayType);
+      for (SizeType i = 0; i < jvm["metrics"].Size(); ++i) {
+        if (strstr(jvm["metrics"][i]["name"].GetString(), "total") != nullptr) 
{
+          total.PushBack(jvm["metrics"][i], document->GetAllocator());
+        } else if (strstr(jvm["metrics"][i]["name"].GetString(), "non-heap") 
!= nullptr) {
+          non_heap.PushBack(jvm["metrics"][i], document->GetAllocator());
+        } else if (strstr(jvm["metrics"][i]["name"].GetString(), "heap") != 
nullptr) {
+          heap.PushBack(jvm["metrics"][i], document->GetAllocator());
+        }
+      }
+      document->AddMember("jvm_total", total, document->GetAllocator());
+      document->AddMember("jvm_heap", heap, document->GetAllocator());
+      document->AddMember("jvm_non_heap", non_heap, document->GetAllocator());
+    }
+    MetricGroup* buffer_pool_group = 
metric_group->FindChildGroup("buffer-pool");
+    if (buffer_pool_group != nullptr) {
+      Value json_metrics(kObjectType);
+      buffer_pool_group->ToJson(false, document, &json_metrics);
+      document->AddMember(
+          "buffer_pool", json_metrics["metrics"], document->GetAllocator());
+    }
+  }
+}
+
+void impala::AddMemUsageCallbacks(Webserver* webserver, MemTracker* 
mem_tracker,
+    MetricGroup* metric_group) {
+  if (mem_tracker != nullptr) {
+    auto callback = [mem_tracker, metric_group]
+        (const Webserver::WebRequest& req, Document* doc) {
+      AddMemTracker(mem_tracker, req, doc);
+      AddTCmallocOverview(req, doc);
+      AddSystemInfo(req, doc);
+      AddMetricGroup(metric_group, req, doc);
+    };
+    webserver->RegisterUrlCallback("/memz", "memz.tmpl", callback, true);
+  } else {
+    auto callback = [metric_group]
+        (const Webserver::WebRequest& req, Document* doc) {
+      AddTCmallocOverview(req, doc);
+      AddSystemInfo(req, doc);
+      AddMetricGroup(metric_group, req, doc);
+    };
+    webserver->RegisterUrlCallback("/memz", "memz.tmpl", callback, true);
+  }
+}
diff --git a/be/src/util/default-path-handlers.h 
b/be/src/util/memusage-path-handlers.h
similarity index 64%
copy from be/src/util/default-path-handlers.h
copy to be/src/util/memusage-path-handlers.h
index 081179b..d5de6b5 100644
--- a/be/src/util/default-path-handlers.h
+++ b/be/src/util/memusage-path-handlers.h
@@ -15,8 +15,8 @@
 // specific language governing permissions and limitations
 // under the License.
 
-#ifndef IMPALA_UTIL_DEFAULT_PATH_HANDLERS_H
-#define IMPALA_UTIL_DEFAULT_PATH_HANDLERS_H
+#ifndef IMPALA_UTIL_MEMUSAGE_PATH_HANDLERS_H
+#define IMPALA_UTIL_MEMUSAGE_PATH_HANDLERS_H
 
 #include <stdio.h>
 #include <rapidjson/document.h>
@@ -28,15 +28,12 @@ namespace impala {
 
 class MemTracker;
 class MetricGroup;
+class Webserver;
 
-/// Adds a set of default path handlers to the webserver to display
-/// logs and configuration flags
-void AddDefaultUrlCallbacks(Webserver* webserver, MemTracker* 
process_mem_tracker = NULL,
-    MetricGroup* metric_group = NULL);
-
-/// Registered to handle "/"
-/// Populates document with various system-wide information.
-void RootHandler(const Webserver::WebRequest& req, rapidjson::Document* 
document);
+/// Creates the memory usage callback for the webserver and registers it. When 
the
+/// mem_tracker is null the mem tracker metrics will not be registered and 
displayed.
+void AddMemUsageCallbacks(Webserver* webserver, MemTracker* mem_tracker,
+    MetricGroup* metric_group);
 }
 
-#endif // IMPALA_UTIL_DEFAULT_PATH_HANDLERS_H
+#endif // IMPALA_UTIL_MEMUSAGE_PATH_HANDLERS_H
diff --git a/www/memz.tmpl b/www/memz.tmpl
index 9c629d8..19ec91b 100644
--- a/www/memz.tmpl
+++ b/www/memz.tmpl
@@ -20,10 +20,14 @@ under the License.
 
 <h2>Memory Usage</h2>
 
+{{#consumption}}
 Memory consumption / limit: <strong>{{consumption}}</strong> / 
<strong>{{mem_limit}}</strong>
+{{/consumption}}
 
+{{#detailed}}
 <h3>Breakdown</h3>
 <pre>{{detailed}}</pre>
+{{/detailed}}
 
 <h3>tcmalloc</h3>
 <pre>{{overview}}</pre>

Reply via email to