Mike Percy has posted comments on this change. Change subject: env_util: Factor out helper DeleteExcessFilesByPattern() ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/5740/2/src/kudu/util/env_util-test.cc File src/kudu/util/env_util-test.cc: Line 29: #include "kudu/util/env_util.h" > Shouldn't this come first though? I think that rule is for the .cc file corresponding to the .h file, to ensure there aren't hidden deps in the .h file (and it should be the very first include, not after system includes). I don't think it helps to additionally do it in the test file. Line 118: TEST_F(EnvUtilTest, TestDeleteExcessFilesByPattern) { > Maybe you could use the utimes() syscall to artificially set the mtime of e Nice! Works great. http://gerrit.cloudera.org:8080/#/c/5740/2/src/kudu/util/env_util.cc File src/kudu/util/env_util.cc: PS2, Line 241: if (max_matches < 0) { : return Status::InvalidArgument("max_matches must be 0 or greater"); : } > Simpler as a DCHECK? The logging.cc call is guaranteed to never provide <0. Done Line 265: for (const auto& matching_file: matching_file_mtimes) { > Nit: "matching_file : matching_file_mtimes" Done -- To view, visit http://gerrit.cloudera.org:8080/5740 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6e76911b0a68f7c1397d93eb027b6a5c99e2fbed Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
