From d77f397b384cbd73e7f18882942dd2900cbe7785 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Fri, 26 Jun 2020 03:16:58 +0300 Subject: [PATCH] review fixes --- programs/server/Server.cpp | 3 +- src/Core/Settings.h | 1 - src/IO/ReadHelpers.h | 3 +- src/Interpreters/AsynchronousMetrics.cpp | 60 ++++++++++++------------ src/Interpreters/AsynchronousMetrics.h | 9 ++-- 5 files changed, 41 insertions(+), 35 deletions(-) diff --git a/programs/server/Server.cpp b/programs/server/Server.cpp index c99543ff631..a0dfb80b037 100644 --- a/programs/server/Server.cpp +++ b/programs/server/Server.cpp @@ -861,7 +861,8 @@ int Server::main(const std::vector & /*args*/) }; /// This object will periodically calculate some metrics. - AsynchronousMetrics async_metrics(*global_context); + AsynchronousMetrics async_metrics(*global_context, + config().getUInt("asynchronous_metrics_update_period_s", 60)); attachSystemTablesAsync(*DatabaseCatalog::instance().getSystemDatabase(), async_metrics); for (const auto & listen_host : listen_hosts) diff --git a/src/Core/Settings.h b/src/Core/Settings.h index 52add89b009..f434132eccd 100644 --- a/src/Core/Settings.h +++ b/src/Core/Settings.h @@ -230,7 +230,6 @@ struct Settings : public SettingsCollection M(SettingUInt64, query_profiler_cpu_time_period_ns, 1000000000, "Period for CPU clock timer of query profiler (in nanoseconds). Set 0 value to turn off the CPU clock query profiler. Recommended value is at least 10000000 (100 times a second) for single queries or 1000000000 (once a second) for cluster-wide profiling.", 0) \ M(SettingBool, metrics_perf_events_enabled, false, "If enabled, some of the perf events will be measured throughout queries' execution.", 0) \ M(SettingString, metrics_perf_events_list, "", "Comma separated list of perf metrics that will be measured throughout queries' execution. Empty means all events. See PerfEventInfo in sources for the available events.", 0) \ - M(SettingUInt64, asynchronous_metrics_update_period_s, 60, "Period for updating asynchronous metrics, in seconds.", 0) \ \ \ /** Limits during query execution are part of the settings. \ diff --git a/src/IO/ReadHelpers.h b/src/IO/ReadHelpers.h index 52601ecb25b..f299ab2286f 100644 --- a/src/IO/ReadHelpers.h +++ b/src/IO/ReadHelpers.h @@ -455,7 +455,8 @@ void readBackQuotedStringWithSQLStyle(String & s, ReadBuffer & buf); void readStringUntilEOF(String & s, ReadBuffer & buf); -// Doesn't read the EOL itself. +// Reads the line until EOL, unescaping backslash escape sequences. +// Buffer pointer is left at EOL, don't forget to advance it. void readEscapedStringUntilEOL(String & s, ReadBuffer & buf); diff --git a/src/Interpreters/AsynchronousMetrics.cpp b/src/Interpreters/AsynchronousMetrics.cpp index 57cc9bce2b8..eb93ac4412b 100644 --- a/src/Interpreters/AsynchronousMetrics.cpp +++ b/src/Interpreters/AsynchronousMetrics.cpp @@ -58,45 +58,44 @@ AsynchronousMetricValues AsynchronousMetrics::getValues() const return values; } +static auto get_next_update_time(std::chrono::seconds update_period) +{ + using namespace std::chrono; + + const auto now = time_point_cast(system_clock::now()); + + // Use seconds since the start of the hour, because we don't know when + // the epoch started, maybe on some weird fractional time. + const auto start_of_hour = time_point_cast(time_point_cast(now)); + const auto seconds_passed = now - start_of_hour; + + // Rotate time forward by half a period -- e.g. if a period is a minute, + // we'll collect metrics on start of minute + 30 seconds. This is to + // achieve temporal separation with MetricTransmitter. Don't forget to + // rotate it back. + const auto rotation = update_period / 2; + + const auto periods_passed = (seconds_passed + rotation) / update_period; + const auto seconds_next = (periods_passed + 1) * update_period - rotation; + const auto time_next = start_of_hour + seconds_next; + + return time_next; +} void AsynchronousMetrics::run() { setThreadName("AsyncMetrics"); - const auto period = std::chrono::seconds( - context.getSettingsRef().asynchronous_metrics_update_period_s); - - const auto get_next_update_time = [period] - { - using namespace std::chrono; - - const auto now = time_point_cast(system_clock::now()); - - // Use seconds since the start of the hour, because we don't know when - // the epoch started, maybe on some weird fractional time. - const auto start_of_hour = time_point_cast(time_point_cast(now)); - const auto seconds_passed = now - start_of_hour; - - // Rotate time forward by half a period -- e.g. if a period is a minute, - // we'll collect metrics on start of minute + 30 seconds. This is to - // achieve temporal separation with MetricTransmitter. Don't forget to - // rotate it back. - const auto rotation = period / 2; - - const auto periods_passed = (seconds_passed + rotation) / period; - const auto seconds_next = (periods_passed + 1) * period - rotation; - const auto time_next = start_of_hour + seconds_next; - - return time_next; - }; - while (true) { { // Wait first, so that the first metric collection is also on even time. std::unique_lock lock{mutex}; - if (wait_cond.wait_until(lock, get_next_update_time(), [this] { return quit; })) + if (wait_cond.wait_until(lock, get_next_update_time(update_period), + [this] { return quit; })) + { break; + } } try @@ -329,7 +328,7 @@ void AsynchronousMetrics::update() // Try to add processor frequencies, ignoring errors. try { - ReadBufferFromFile buf("/proc/cpuinfo"); + ReadBufferFromFile buf("/proc/cpuinfo", 32768 /* buf_size */); // We need the following lines: // core id : 4 @@ -339,6 +338,9 @@ void AsynchronousMetrics::update() while (!buf.eof()) { std::string s; + // We don't have any backslash escape sequences in /proc/cpuinfo, so + // this function will read the line until EOL, which is exactly what + // we need. readEscapedStringUntilEOL(s, buf); // It doesn't read the EOL itself. ++buf.position(); diff --git a/src/Interpreters/AsynchronousMetrics.h b/src/Interpreters/AsynchronousMetrics.h index 6817f545c8f..dc892b9d40c 100644 --- a/src/Interpreters/AsynchronousMetrics.h +++ b/src/Interpreters/AsynchronousMetrics.h @@ -18,15 +18,17 @@ typedef double AsynchronousMetricValue; typedef std::unordered_map AsynchronousMetricValues; -/** Periodically (each minute, starting at 30 seconds offset) +/** Periodically (by default, each minute, starting at 30 seconds offset) * calculates and updates some metrics, * that are not updated automatically (so, need to be asynchronously calculated). */ class AsynchronousMetrics { public: - AsynchronousMetrics(Context & context_) - : context(context_), thread([this] { run(); }) + AsynchronousMetrics(Context & context_, int update_period_seconds) + : context(context_), + update_period(update_period_seconds), + thread([this] { run(); }) { } @@ -38,6 +40,7 @@ public: private: Context & context; + const std::chrono::seconds update_period; mutable std::mutex mutex; std::condition_variable wait_cond;