Dan Burkert has posted comments on this change. Change subject: KUDU-766: limit number of glog files ......................................................................
Patch Set 2: (14 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. Do you mind if I hold off on this? It's not immediately clear what the right abstraction is. I started implementing class ExternalDaemon { virtual static daemon_id(int32_t id) const; } But there are a couple of issues: 1) virtual and static can't mix, so it would have to be defined on each of the subclasses. 2) This API can only be used if the caller assumes the IDs are consecutive integers starting at 0, so it might be hiding one aspect of external mini cluster implementation (the string format), but it still assumes a knowledge of another aspect (the ID generation pattern). 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 on Done PS1, Line 334: Substitute("ts-$0", idx); > Encapsulate. same. http://gerrit.cloudera.org:8080/#/c/5340/2/src/kudu/integration-tests/external_mini_cluster.cc File src/kudu/integration-tests/external_mini_cluster.cc: Line 232: } else { > warning: don't use else after return [readability-else-after-return] Done http://gerrit.cloudera.org:8080/#/c/5340/2/src/kudu/integration-tests/external_mini_cluster.h File src/kudu/integration-tests/external_mini_cluster.h: Line 376: const boost::optional<string>& log_dir() const { > error: use of undeclared identifier 'string'; did you mean 'strings'? [clan Done Line 377: return log_dir_; > error: no viable conversion from returned value of type 'const boost::optio not sure what this means; going to assume it's because I messed up the namespacing. 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. Done Line 49: CHECK_OK(cluster.Start()); > These should all be ASSERT_OK. Done 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 e Yes, this would warn every 60 seconds. Note that StartExcessGlogDeleterThread calls this inline with the server startup, so if the permissions are bad server startup will fail. The permissions would have to change at runtime in order to get failure. Line 362: if (metrics_logging_thread_) { > This needs to be reworked to account for the new thread. Reworked in what way? I looked at this when I first made the change to see if I needed to initialize the count to two instead of one, but I don't think that's necessary. There's still only one context shutting down the server, regardless of the number of background threads waiting on the latch. http://gerrit.cloudera.org:8080/#/c/5340/1/src/kudu/util/logging.cc File src/kudu/util/logging.cc: Line 319: // Ignore bad input or disable log rotation. > How about passing in an Env* as an argument? One can be had from ServerBase Done PS1, Line 325: > These aren't modifiable at runtime, right? No, these are both used in the glog initialization so I don't think they can be changed (plus they are strings, so there would be thread safety issues). max_log_files is set to be modifiable, since I didn't see a downside to doing so. Line 333: continue; > Aren't glog filenames terminated with a date/time stamp, with second-level Yes, I think it would. I think we should stick with mtimes for now, since that's what Impala uses, and it seems to be working for them. I'll add a note to that effect. 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" (mtim Done -- 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: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
