You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by to...@apache.org on 2018/02/03 01:09:27 UTC

[1/4] kudu git commit: Fix log message when there's too few data dirs for a data dir group

Repository: kudu
Updated Branches:
  refs/heads/master 7b6cc8459 -> 2bff29b59


Fix log message when there's too few data dirs for a data dir group

The address of the dirs full and dirs failed metrics were being
logged instead of the metric value:

I0201 14:45:22.566895  3316 data_dirs.cc:930] Could only allocate 1 dirs of requested 3 for tablet 78a4a8db9829474e84b7a5474c9114fd. 1 dirs total, 0x379c750 dirs full, 0x37599b0 dirs failed

Change-Id: I877d335a75625a087e4e9027b7f311fc8c0805c9
Reviewed-on: http://gerrit.cloudera.org:8080/9194
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>


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

Branch: refs/heads/master
Commit: 6d8694cac2a601e97693afc81eb01bf189b7886f
Parents: 7b6cc84
Author: Will Berkeley <wd...@apache.org>
Authored: Fri Feb 2 10:29:31 2018 -0800
Committer: Andrew Wong <aw...@cloudera.com>
Committed: Fri Feb 2 22:35:06 2018 +0000

----------------------------------------------------------------------
 src/kudu/fs/data_dirs.cc | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/6d8694ca/src/kudu/fs/data_dirs.cc
----------------------------------------------------------------------
diff --git a/src/kudu/fs/data_dirs.cc b/src/kudu/fs/data_dirs.cc
index 163cec7..5b7e4cd 100644
--- a/src/kudu/fs/data_dirs.cc
+++ b/src/kudu/fs/data_dirs.cc
@@ -126,6 +126,7 @@ using std::unordered_map;
 using std::unordered_set;
 using std::vector;
 using strings::Substitute;
+using strings::SubstituteAndAppend;
 
 
 namespace {
@@ -924,10 +925,10 @@ Status DataDirManager::CreateDataDirGroup(const string& tablet_id,
                               "$2. $3 dirs total", group_indices.size(),
                               FLAGS_fs_target_data_dirs_per_tablet, tablet_id, data_dirs_.size());
       if (metrics_) {
-        msg = Substitute("$0, $1 dirs full, $2 dirs failed", msg,
-                         metrics_->data_dirs_full.get(), metrics_->data_dirs_failed.get());
+        SubstituteAndAppend(&msg, ", $0 dirs full, $1 dirs failed",
+                            metrics_->data_dirs_full->value(), metrics_->data_dirs_failed->value());
       }
-      LOG(INFO) << Substitute(msg);
+      LOG(INFO) << msg;
     }
   }
   InsertOrDie(&group_by_tablet_map_, tablet_id, DataDirGroup(group_indices));


[4/4] kudu git commit: jsonwriter: do some internal buffering in front of ostringstream

Posted by to...@apache.org.
jsonwriter: do some internal buffering in front of ostringstream

JsonWriter outputs into a std::ostringstream, and the RapidJSON
interface is such that we always append one character at a time.
ostringstream::put(char) is not inlined, so this is very slow.

This patch adds a small buffer in front of the ostringstream. We flush
the buffer on the end of any JSON array or object.

The patch adds a simple benchmark to show the improvement:

Before:
I0131 20:26:09.437477 16201 jsonwriter-test.cc:187] Throughput: 87.0644MB/sec

After:
I0131 20:24:17.889117 15929 jsonwriter-test.cc:187] Throughput: 154.192MB/sec

The remaining perf issues seem to be due to our use of protobuf
reflection.

Change-Id: Ib45d58187cf80ecfe2f5a25e40d7042e4a27619d
Reviewed-on: http://gerrit.cloudera.org:8080/9181
Tested-by: Kudu Jenkins
Reviewed-by: Will Berkeley <wd...@gmail.com>


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

Branch: refs/heads/master
Commit: 2bff29b5949825515dfa827a98973a9dbdc32b86
Parents: afae5c7
Author: Todd Lipcon <to...@apache.org>
Authored: Wed Jan 31 20:24:26 2018 -0800
Committer: Todd Lipcon <to...@apache.org>
Committed: Sat Feb 3 01:08:49 2018 +0000

----------------------------------------------------------------------
 src/kudu/util/jsonwriter-test.cc | 72 ++++++++++++++++++++++++++---------
 src/kudu/util/jsonwriter.cc      | 26 +++++++++++--
 2 files changed, 77 insertions(+), 21 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/2bff29b5/src/kudu/util/jsonwriter-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/jsonwriter-test.cc b/src/kudu/util/jsonwriter-test.cc
index 9dc7e69..608d9c0 100644
--- a/src/kudu/util/jsonwriter-test.cc
+++ b/src/kudu/util/jsonwriter-test.cc
@@ -15,19 +15,48 @@
 // specific language governing permissions and limitations
 // under the License.
 
+#include <ostream>
+#include <stdint.h>
+#include <string>
+
 #include <gflags/gflags.h>
+#include <glog/logging.h>
 #include <gtest/gtest.h>
 
 #include "kudu/gutil/strings/substitute.h"
 #include "kudu/util/jsonwriter.h"
 #include "kudu/util/jsonwriter_test.pb.h"
+#include "kudu/util/stopwatch.h"
 #include "kudu/util/test_util.h"
 
 using jsonwriter_test::TestAllTypes;
 
 namespace kudu {
 
-class TestJsonWriter : public KuduTest {};
+class TestJsonWriter : public KuduTest {
+ protected:
+  TestAllTypes MakeAllTypesPB() {
+    TestAllTypes pb;
+    pb.set_optional_int32(1);
+    pb.set_optional_int64(2);
+    pb.set_optional_uint32(3);
+    pb.set_optional_uint64(4);
+    pb.set_optional_sint32(5);
+    pb.set_optional_sint64(6);
+    pb.set_optional_fixed32(7);
+    pb.set_optional_fixed64(8);
+    pb.set_optional_sfixed32(9);
+    pb.set_optional_sfixed64(10);
+    pb.set_optional_float(11);
+    pb.set_optional_double(12);
+    pb.set_optional_bool(true);
+    pb.set_optional_string("hello world");
+    pb.set_optional_redacted_string("secret!");
+    pb.set_optional_nested_enum(TestAllTypes::FOO);
+    return pb;
+  }
+
+};
 
 TEST_F(TestJsonWriter, TestPBEmpty) {
   TestAllTypes pb;
@@ -36,23 +65,8 @@ TEST_F(TestJsonWriter, TestPBEmpty) {
 
 TEST_F(TestJsonWriter, TestPBAllFieldTypes) {
   ASSERT_NE("", gflags::SetCommandLineOption("redact", "log"));
-  TestAllTypes pb;
-  pb.set_optional_int32(1);
-  pb.set_optional_int64(2);
-  pb.set_optional_uint32(3);
-  pb.set_optional_uint64(4);
-  pb.set_optional_sint32(5);
-  pb.set_optional_sint64(6);
-  pb.set_optional_fixed32(7);
-  pb.set_optional_fixed64(8);
-  pb.set_optional_sfixed32(9);
-  pb.set_optional_sfixed64(10);
-  pb.set_optional_float(11);
-  pb.set_optional_double(12);
-  pb.set_optional_bool(true);
-  pb.set_optional_string("hello world");
-  pb.set_optional_redacted_string("secret!");
-  pb.set_optional_nested_enum(TestAllTypes::FOO);
+  TestAllTypes pb = MakeAllTypesPB();
+
   ASSERT_EQ("{\n"
             "    \"optional_int32\": 1,\n"
             "    \"optional_int64\": 2,\n"
@@ -156,4 +170,26 @@ TEST_F(TestJsonWriter, TestPBNestedMessage) {
             JsonWriter::ToJson(pb, JsonWriter::COMPACT));
 }
 
+TEST_F(TestJsonWriter, Benchmark) {
+  TestAllTypes pb = MakeAllTypesPB();
+
+  int64_t total_len = 0;
+  Stopwatch sw;
+  sw.start();
+  while (sw.elapsed().wall_seconds() < 5) {
+    std::ostringstream str;
+    JsonWriter jw(&str, JsonWriter::COMPACT);
+    jw.StartArray();
+    for (int i = 0; i < 10000; i++) {
+      jw.Protobuf(pb);
+    }
+    jw.EndArray();
+    total_len += str.str().size();
+  }
+  sw.stop();
+  double mbps = total_len / 1024.0 / 1024.0 / sw.elapsed().user_cpu_seconds();
+  LOG(INFO) << "Throughput: " << mbps << "MB/sec";
+}
+
+
 } // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/2bff29b5/src/kudu/util/jsonwriter.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/jsonwriter.cc b/src/kudu/util/jsonwriter.cc
index d2f1f65..f194d5b 100644
--- a/src/kudu/util/jsonwriter.cc
+++ b/src/kudu/util/jsonwriter.cc
@@ -31,6 +31,7 @@
 #include <rapidjson/rapidjson.h>
 
 #include "kudu/gutil/port.h"
+#include "kudu/util/faststring.h"
 #include "kudu/util/logging.h"
 #include "kudu/util/pb_util.pb.h"
 
@@ -49,8 +50,13 @@ namespace kudu {
 class UTF8StringStreamBuffer {
  public:
   explicit UTF8StringStreamBuffer(std::ostringstream* out);
+  ~UTF8StringStreamBuffer();
   void Put(rapidjson::UTF8<>::Ch c);
+
+  void Flush();
+
  private:
+  faststring buf_;
   std::ostringstream* out_;
 };
 
@@ -285,9 +291,17 @@ string JsonWriter::ToJson(const Message& pb, Mode mode) {
 UTF8StringStreamBuffer::UTF8StringStreamBuffer(std::ostringstream* out)
   : out_(DCHECK_NOTNULL(out)) {
 }
+UTF8StringStreamBuffer::~UTF8StringStreamBuffer() {
+  DCHECK_EQ(buf_.size(), 0) << "Forgot to flush!";
+}
 
 void UTF8StringStreamBuffer::Put(rapidjson::UTF8<>::Ch c) {
-  out_->put(c);
+  buf_.push_back(c);
+}
+
+void UTF8StringStreamBuffer::Flush() {
+  out_->write(reinterpret_cast<char*>(buf_.data()), buf_.size());
+  buf_.clear();
 }
 
 //
@@ -322,10 +336,16 @@ void JsonWriterImpl<T>::String(const string& str) { writer_.String(str.c_str(),
 template<class T>
 void JsonWriterImpl<T>::StartObject() { writer_.StartObject(); }
 template<class T>
-void JsonWriterImpl<T>::EndObject() { writer_.EndObject(); }
+void JsonWriterImpl<T>::EndObject() {
+  writer_.EndObject();
+  stream_.Flush();
+}
 template<class T>
 void JsonWriterImpl<T>::StartArray() { writer_.StartArray(); }
 template<class T>
-void JsonWriterImpl<T>::EndArray() { writer_.EndArray(); }
+void JsonWriterImpl<T>::EndArray() {
+  writer_.EndArray();
+  stream_.Flush();
+}
 
 } // namespace kudu


[3/4] kudu git commit: metrics: fast-path PB/JSON dump for empty histograms

Posted by to...@apache.org.
metrics: fast-path PB/JSON dump for empty histograms

It turns out that a decent percentage of our histograms are empty in
real life use cases. For example, some tablets might be cold enough that
they have never had any action since the tserver started. Other
histograms might correspond to features which aren't being used in a
particular workload (eg commit-wait counters).

For example, in a test cluster running a cycling YCSB workload, about
20% of histograms are empty. In another test cluster running more of an
analytic workload with a lot of very cold tablets, 81% of histograms
were empty.

We can fast-path various calculations on these histograms to just return
0 for a simple speedup on the /metricsjson endpoint.

Change-Id: I0732e8299e2f27c7c6021ec70740984ab46a69c0
Reviewed-on: http://gerrit.cloudera.org:8080/9179
Tested-by: Kudu Jenkins
Reviewed-by: Will Berkeley <wd...@gmail.com>


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

Branch: refs/heads/master
Commit: afae5c75112c9938ed9f2b335a75a92ea63f5587
Parents: 3e49fe2
Author: Todd Lipcon <to...@apache.org>
Authored: Wed Jan 31 18:02:03 2018 -0800
Committer: Todd Lipcon <to...@apache.org>
Committed: Sat Feb 3 01:08:32 2018 +0000

----------------------------------------------------------------------
 src/kudu/util/metrics.cc | 58 +++++++++++++++++++++++++++----------------
 src/kudu/util/metrics.h  |  2 +-
 2 files changed, 38 insertions(+), 22 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/afae5c75/src/kudu/util/metrics.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/metrics.cc b/src/kudu/util/metrics.cc
index 4954bac..6565d97 100644
--- a/src/kudu/util/metrics.cc
+++ b/src/kudu/util/metrics.cc
@@ -627,34 +627,50 @@ Status Histogram::WriteAsJson(JsonWriter* writer,
 
 Status Histogram::GetHistogramSnapshotPB(HistogramSnapshotPB* snapshot_pb,
                                          const MetricJsonOptions& opts) const {
-  HdrHistogram snapshot(*histogram_);
   snapshot_pb->set_name(prototype_->name());
   if (opts.include_schema_info) {
     snapshot_pb->set_type(MetricType::Name(prototype_->type()));
     snapshot_pb->set_label(prototype_->label());
     snapshot_pb->set_unit(MetricUnit::Name(prototype_->unit()));
     snapshot_pb->set_description(prototype_->description());
-    snapshot_pb->set_max_trackable_value(snapshot.highest_trackable_value());
-    snapshot_pb->set_num_significant_digits(snapshot.num_significant_digits());
+    snapshot_pb->set_max_trackable_value(histogram_->highest_trackable_value());
+    snapshot_pb->set_num_significant_digits(histogram_->num_significant_digits());
   }
-  snapshot_pb->set_total_count(snapshot.TotalCount());
-  snapshot_pb->set_total_sum(snapshot.TotalSum());
-  snapshot_pb->set_min(snapshot.MinValue());
-  snapshot_pb->set_mean(snapshot.MeanValue());
-  snapshot_pb->set_percentile_75(snapshot.ValueAtPercentile(75));
-  snapshot_pb->set_percentile_95(snapshot.ValueAtPercentile(95));
-  snapshot_pb->set_percentile_99(snapshot.ValueAtPercentile(99));
-  snapshot_pb->set_percentile_99_9(snapshot.ValueAtPercentile(99.9));
-  snapshot_pb->set_percentile_99_99(snapshot.ValueAtPercentile(99.99));
-  snapshot_pb->set_max(snapshot.MaxValue());
-
-  if (opts.include_raw_histograms) {
-    RecordedValuesIterator iter(&snapshot);
-    while (iter.HasNext()) {
-      HistogramIterationValue value;
-      RETURN_NOT_OK(iter.Next(&value));
-      snapshot_pb->add_values(value.value_iterated_to);
-      snapshot_pb->add_counts(value.count_at_value_iterated_to);
+  // Fast-path for a reasonably common case of an empty histogram. This occurs
+  // when a histogram is tracking some information about a feature not in
+  // use, for example.
+  if (histogram_->TotalCount() == 0) {
+    snapshot_pb->set_total_count(0);
+    snapshot_pb->set_total_sum(0);
+    snapshot_pb->set_min(0);
+    snapshot_pb->set_mean(0);
+    snapshot_pb->set_percentile_75(0);
+    snapshot_pb->set_percentile_95(0);
+    snapshot_pb->set_percentile_99(0);
+    snapshot_pb->set_percentile_99_9(0);
+    snapshot_pb->set_percentile_99_99(0);
+    snapshot_pb->set_max(0);
+  } else {
+    HdrHistogram snapshot(*histogram_);
+    snapshot_pb->set_total_count(snapshot.TotalCount());
+    snapshot_pb->set_total_sum(snapshot.TotalSum());
+    snapshot_pb->set_min(snapshot.MinValue());
+    snapshot_pb->set_mean(snapshot.MeanValue());
+    snapshot_pb->set_percentile_75(snapshot.ValueAtPercentile(75));
+    snapshot_pb->set_percentile_95(snapshot.ValueAtPercentile(95));
+    snapshot_pb->set_percentile_99(snapshot.ValueAtPercentile(99));
+    snapshot_pb->set_percentile_99_9(snapshot.ValueAtPercentile(99.9));
+    snapshot_pb->set_percentile_99_99(snapshot.ValueAtPercentile(99.99));
+    snapshot_pb->set_max(snapshot.MaxValue());
+
+    if (opts.include_raw_histograms) {
+      RecordedValuesIterator iter(&snapshot);
+      while (iter.HasNext()) {
+        HistogramIterationValue value;
+        RETURN_NOT_OK(iter.Next(&value));
+        snapshot_pb->add_values(value.value_iterated_to);
+        snapshot_pb->add_counts(value.count_at_value_iterated_to);
+      }
     }
   }
   return Status::OK();

http://git-wip-us.apache.org/repos/asf/kudu/blob/afae5c75/src/kudu/util/metrics.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/metrics.h b/src/kudu/util/metrics.h
index 60412c2..aa553c9 100644
--- a/src/kudu/util/metrics.h
+++ b/src/kudu/util/metrics.h
@@ -1009,7 +1009,7 @@ class Histogram : public Metric {
                              const MetricJsonOptions& opts) const OVERRIDE;
 
   // Returns a snapshot of this histogram including the bucketed values and counts.
-  Status GetHistogramSnapshotPB(HistogramSnapshotPB* snapshot,
+  Status GetHistogramSnapshotPB(HistogramSnapshotPB* snapshot_pb,
                                 const MetricJsonOptions& opts) const;
 
   uint64_t CountInBucketForValueForTests(uint64_t value) const;


[2/4] kudu git commit: KUDU-2279 (part 1). rolling_log: respect logfile retention flag

Posted by to...@apache.org.
KUDU-2279 (part 1). rolling_log: respect logfile retention flag

Previously we never deleted old metrics logs, making it somewhat
dangerous to enable this in production. Now we respect the same
retention gflag that we apply to our other glogs.

Change-Id: If3cc9b085c9827fd544b0bf7d7ae868e3692296b
Reviewed-on: http://gerrit.cloudera.org:8080/9175
Tested-by: Kudu Jenkins
Reviewed-by: Will Berkeley <wd...@gmail.com>


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

Branch: refs/heads/master
Commit: 3e49fe2f7ca9afa603da600872cf3649346a0c22
Parents: 6d8694c
Author: Todd Lipcon <to...@apache.org>
Authored: Wed Jan 31 16:11:44 2018 -0800
Committer: Todd Lipcon <to...@apache.org>
Committed: Sat Feb 3 00:57:34 2018 +0000

----------------------------------------------------------------------
 src/kudu/util/rolling_log-test.cc | 15 ++++++
 src/kudu/util/rolling_log.cc      | 87 +++++++++++++++++++++-------------
 src/kudu/util/rolling_log.h       | 16 +++++--
 3 files changed, 81 insertions(+), 37 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/3e49fe2f/src/kudu/util/rolling_log-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/rolling_log-test.cc b/src/kudu/util/rolling_log-test.cc
index 626c0de..5c4c13b 100644
--- a/src/kudu/util/rolling_log-test.cc
+++ b/src/kudu/util/rolling_log-test.cc
@@ -125,4 +125,19 @@ TEST_F(RollingLogTest, TestCompression) {
   ASSERT_GT(size, 0);
 }
 
+TEST_F(RollingLogTest, TestFileCountLimit) {
+  RollingLog log(env_, log_dir_, "mylog");
+  ASSERT_OK(log.Open());
+  log.SetSizeLimitBytes(100);
+  log.SetMaxNumSegments(3);
+
+  for (int i = 0; i < 100; i++) {
+    ASSERT_OK(log.Append("hello world\n"));
+  }
+  ASSERT_OK(log.Close());
+
+  vector<string> children;
+  NO_FATALS(AssertLogCount(3, &children));
+}
+
 } // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/3e49fe2f/src/kudu/util/rolling_log.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/rolling_log.cc b/src/kudu/util/rolling_log.cc
index 9f8d10f..a508e91 100644
--- a/src/kudu/util/rolling_log.cc
+++ b/src/kudu/util/rolling_log.cc
@@ -27,6 +27,7 @@
 #include <utility>
 
 #include <gflags/gflags.h>
+#include <gflags/gflags_declare.h>
 #include <glog/logging.h>
 #include <zlib.h>
 
@@ -34,6 +35,7 @@
 #include "kudu/gutil/strings/substitute.h"
 #include "kudu/gutil/walltime.h"
 #include "kudu/util/env.h"
+#include "kudu/util/env_util.h"
 #include "kudu/util/net/net_util.h"
 #include "kudu/util/path_util.h"
 #include "kudu/util/slice.h"
@@ -47,6 +49,8 @@ using strings::Substitute;
 
 static const int kDefaultSizeLimitBytes = 64 * 1024 * 1024; // 64MB
 
+DECLARE_int32(max_log_files);
+
 namespace kudu {
 
 RollingLog::RollingLog(Env* env, string log_dir, string log_name)
@@ -54,6 +58,7 @@ RollingLog::RollingLog(Env* env, string log_dir, string log_name)
       log_dir_(std::move(log_dir)),
       log_name_(std::move(log_name)),
       size_limit_bytes_(kDefaultSizeLimitBytes),
+      max_num_segments_(FLAGS_max_log_files),
       compress_after_close_(true) {}
 
 RollingLog::~RollingLog() {
@@ -65,42 +70,42 @@ void RollingLog::SetSizeLimitBytes(int64_t size) {
   size_limit_bytes_ = size;
 }
 
+void RollingLog::SetMaxNumSegments(int num_segments) {
+  CHECK_GT(num_segments, 0);
+  max_num_segments_ = num_segments;
+}
+
 void RollingLog::SetCompressionEnabled(bool compress) {
   compress_after_close_ = compress;
 }
 
-string RollingLog::GetLogFileName(int sequence) const {
-  ostringstream str;
-
-  // 1. Program name.
-  str << google::ProgramInvocationShortName();
+namespace {
 
-  // 2. Host name.
+string HostnameOrUnknown() {
   string hostname;
   Status s = GetHostname(&hostname);
   if (!s.ok()) {
-    hostname = "unknown_host";
+    return "unknown_host";
   }
-  str << "." << hostname;
+  return hostname;
+}
 
-  // 3. User name.
+string UsernameOrUnknown() {
   string user_name;
-  s = GetLoggedInUser(&user_name);
+  Status s = GetLoggedInUser(&user_name);
   if (!s.ok()) {
-    user_name = "unknown_user";
+    return "unknown_user";
   }
-  str << "." << user_name;
-
-  // 4. Log name.
-  str << "." << log_name_;
+  return user_name;
+}
 
-  // 5. Timestamp.
+string FormattedTimestamp() {
   // Implementation cribbed from glog/logging.cc
   time_t time = static_cast<time_t>(WallTime_Now());
   struct ::tm tm_time;
   localtime_r(&time, &tm_time);
 
-  str << ".";
+  ostringstream str;
   str.fill('0');
   str << 1900+tm_time.tm_year
       << setw(2) << 1+tm_time.tm_mon
@@ -109,15 +114,31 @@ string RollingLog::GetLogFileName(int sequence) const {
       << setw(2) << tm_time.tm_hour
       << setw(2) << tm_time.tm_min
       << setw(2) << tm_time.tm_sec;
-  str.clear(); // resets formatting flags
+  return str.str();
+}
 
-  // 6. Sequence number.
-  str << "." << sequence;
+} // anonymous namespace
 
-  // 7. Pid.
-  str << "." << getpid();
+string RollingLog::GetLogFileName(int sequence) const {
+  return Substitute("$0.$1.$2.$3.$4.$5.$6",
+                    google::ProgramInvocationShortName(),
+                    HostnameOrUnknown(),
+                    UsernameOrUnknown(),
+                    log_name_,
+                    FormattedTimestamp(),
+                    sequence,
+                    getpid());
+}
 
-  return str.str();
+string RollingLog::GetLogFilePattern() const {
+  return Substitute("$0.$1.$2.$3.$4.$5.$6",
+                    google::ProgramInvocationShortName(),
+                    HostnameOrUnknown(),
+                    UsernameOrUnknown(),
+                    log_name_,
+                    /* any timestamp */'*',
+                    /* any sequence number */'*',
+                    /* any pid */'*');
 }
 
 Status RollingLog::Open() {
@@ -125,21 +146,20 @@ Status RollingLog::Open() {
 
   for (int sequence = 0; ; sequence++) {
 
-    string path = JoinPathSegments(log_dir_,
-                                   GetLogFileName(sequence));
+    string path = JoinPathSegments(log_dir_, GetLogFileName(sequence));
+    // Don't reuse an existing path if there is already a log
+    // or a compressed log with the same name.
+    if (env_->FileExists(path) ||
+        env_->FileExists(path + ".gz")) {
+      continue;
+    }
 
     WritableFileOptions opts;
     // Logs aren't worth the performance cost of durability.
     opts.sync_on_close = false;
     opts.mode = Env::CREATE_NON_EXISTING;
 
-    Status s = env_->NewWritableFile(opts, path, &file_);
-    if (s.IsAlreadyPresent()) {
-      // We already rolled once at this same timestamp.
-      // Try again with a new sequence number.
-      continue;
-    }
-    RETURN_NOT_OK(s);
+    RETURN_NOT_OK(env_->NewWritableFile(opts, path, &file_));
 
     VLOG(1) << "Rolled " << log_name_ << " log to new file: " << path;
     break;
@@ -158,6 +178,9 @@ Status RollingLog::Close() {
   if (compress_after_close_) {
     WARN_NOT_OK(CompressFile(path), "Unable to compress old log file");
   }
+  auto glob = JoinPathSegments(log_dir_, GetLogFilePattern());
+  WARN_NOT_OK(env_util::DeleteExcessFilesByPattern(env_, glob, max_num_segments_),
+              Substitute("failed to delete old $0 log files", log_name_));
   return Status::OK();
 }
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/3e49fe2f/src/kudu/util/rolling_log.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/rolling_log.h b/src/kudu/util/rolling_log.h
index 01829f0..897572a 100644
--- a/src/kudu/util/rolling_log.h
+++ b/src/kudu/util/rolling_log.h
@@ -63,11 +63,13 @@ class RollingLog {
   // the log as necessary if it is not open.
   Status Open();
 
-  // Set the size limit for the current and any future log files.
-  //
-  // There is no limit on the total number of previous log segments. We rely
-  // on system utilities to clean up old logs to maintain some size limit.
-  void SetSizeLimitBytes(int64_t bytes);
+  // Set the pre-compression size limit for the current and any future log files.
+  // Note that this is the limit on a single segment of the log, not the total.
+  void SetSizeLimitBytes(int64_t size);
+
+  // Set the total number of log segments to be retained. When the log is rolled,
+  // old segments are removed to achieve the targeted number of segments.
+  void SetMaxNumSegments(int num_segments);
 
   // If compression is enabled, log files are compressed.
   // NOTE: this requires that the passed-in Env instance is the local file system.
@@ -89,6 +91,9 @@ class RollingLog {
  private:
   std::string GetLogFileName(int sequence) const;
 
+  // Get a glob pattern matching all log files written by this instance.
+  std::string GetLogFilePattern() const;
+
   // Compress the given path, writing a new file '<path>.gz'.
   Status CompressFile(const std::string& path) const;
 
@@ -97,6 +102,7 @@ class RollingLog {
   const std::string log_name_;
 
   int64_t size_limit_bytes_;
+  int max_num_segments_;
 
   std::unique_ptr<WritableFile> file_;
   bool compress_after_close_;