You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2017/01/19 08:22:01 UTC

[kudu-CR] env util: Factor out helper DeleteExcessFilesByPattern()

Hello Adar Dembo,

I'd like you to do a code review.  Please visit

    http://gerrit.cloudera.org:8080/5740

to review the following change.

Change subject: env_util: Factor out helper DeleteExcessFilesByPattern()
......................................................................

env_util: Factor out helper DeleteExcessFilesByPattern()

The logic contained in here is generally useful for log rotation
purposes, and we can reuse it for minidump rotation in a later patch.

Change-Id: I6e76911b0a68f7c1397d93eb027b6a5c99e2fbed
---
M src/kudu/util/env_util-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/logging.cc
4 files changed, 81 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/5740/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5740
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6e76911b0a68f7c1397d93eb027b6a5c99e2fbed
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>

[kudu-CR] env util: Factor out helper DeleteExcessFilesByPattern()

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has submitted this change and it was merged.

Change subject: env_util: Factor out helper DeleteExcessFilesByPattern()
......................................................................


env_util: Factor out helper DeleteExcessFilesByPattern()

The logic contained in here is generally useful for log rotation
purposes, and we can reuse it for minidump rotation in a later patch.

Change-Id: I6e76911b0a68f7c1397d93eb027b6a5c99e2fbed
Reviewed-on: http://gerrit.cloudera.org:8080/5740
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/util/env_util-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/logging.cc
4 files changed, 81 insertions(+), 26 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/5740
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I6e76911b0a68f7c1397d93eb027b6a5c99e2fbed
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot

[kudu-CR] env util: Factor out helper DeleteExcessFilesByPattern()

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: env_util: Factor out helper DeleteExcessFilesByPattern()
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5740/2/src/kudu/util/env_util-test.cc
File src/kudu/util/env_util-test.cc:

Line 118: TEST_F(EnvUtilTest, TestDeleteExcessFilesByPattern) {
> Nice! Works great.
You accidentally left the AllowSlowTests() and SleepFor() calls in though. Plus you should probably update the main test comment.


-- 
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] env util: Factor out helper DeleteExcessFilesByPattern()

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo 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?


Line 118: TEST_F(EnvUtilTest, TestDeleteExcessFilesByPattern) {
Maybe you could use the utimes() syscall to artificially set the mtime of each file as you see fit? Then the test could behave the same way for slow and fast mode, and you won't need to sleep at all.


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.


Line 265:   for (const auto& matching_file: matching_file_mtimes) {
Nit: "matching_file : matching_file_mtimes"


-- 
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes