Adar Dembo has posted comments on this change. Change subject: KUDU-766: limit number of glog files ......................................................................
Patch Set 1: (12 comments) http://gerrit.cloudera.org:8080/#/c/5340/1/src/kudu/integration-tests/external_mini_cluster.cc File src/kudu/integration-tests/external_mini_cluster.cc: PS1, Line 293: Substitute("master-$0", i); Let's encapsulate this somewhere. It's also used by one or two tests. PS1, Line 295: boost::optional<string> log_dir = GetLogPath(daemon_id); : if (log_dir) { : RETURN_NOT_OK(Env::Default()->CreateDir(*log_dir)); : } Since we're doing the same thing for masters/tservers, can we do it just once in ExternalDaemon? PS1, Line 334: Substitute("ts-$0", idx); Encapsulate. http://gerrit.cloudera.org:8080/#/c/5340/1/src/kudu/integration-tests/log-rolling-itest.cc File src/kudu/integration-tests/log-rolling-itest.cc: PS1, Line 34: vector Should #include and "using" std::vector. Line 49: CHECK_OK(cluster.Start()); These should all be ASSERT_OK. http://gerrit.cloudera.org:8080/#/c/5340/1/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: Line 328: WARN_NOT_OK(DeleteExcessLogFiles(), "Unable to delete excess log files"); If we actually do warn something, we're likely to repeatedly warn it with every invocation. Maybe using some variant of K_LOG_EVERY_FOO() is in order? Line 362: if (metrics_logging_thread_) { This needs to be reworked to account for the new thread. http://gerrit.cloudera.org:8080/#/c/5340/1/src/kudu/util/logging.cc File src/kudu/util/logging.cc: Line 319: Env* env = Env::Default(); How about passing in an Env* as an argument? One can be had from ServerBase, or from any KuduTest. PS1, Line 325: FLAGS_log_dir, FLAGS_log_filename These aren't modifiable at runtime, right? Line 333: vector<pair<time_t, string>> logfile_mtimes; Aren't glog filenames terminated with a date/time stamp, with second-level granularity? Would that be sufficient for determining which log files should be deleted? http://gerrit.cloudera.org:8080/#/c/5340/1/src/kudu/util/logging.h File src/kudu/util/logging.h: Line 216: // Deletes excess rotated log files. Would be nice to specify here what we're using to figure out "excess" (mtime, filename, etc). http://gerrit.cloudera.org:8080/#/c/5340/1/src/kudu/util/metrics.h File src/kudu/util/metrics.h: Line 27: // This API provides a basic set of metrics primitives along the lines of the Coda Hale's Nice. -- To view, visit http://gerrit.cloudera.org:8080/5340 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia3f81638a6dfcf134665322f091ff26d29640fd9 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
