review fixes

programs/server/Server.cpp

@@ -861,7 +861,8 @@ int Server::main(const std::vector<std::string> & /*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)

src/Core/Settings.h

@@ -230,7 +230,6 @@ struct Settings : public SettingsCollection<Settings>
     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. \

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);
 
 

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<seconds>(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<seconds>(time_point_cast<hours>(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<seconds>(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<seconds>(time_point_cast<hours>(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();

src/Interpreters/AsynchronousMetrics.h

@@ -18,15 +18,17 @@ typedef double AsynchronousMetricValue;
 typedef std::unordered_map<std::string, AsynchronousMetricValue> 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_)
+    AsynchronousMetrics(Context & context_, int update_period_seconds)
-        : context(context_), thread([this] { run(); })
+        : 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;