[kudu-CR] [Log] KUDU-3369 Support compress Kudu log and use size and number of files to manage

["Alexey Serbin (Code Review)" <ge...@cloudera.org>]

Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18554 )

Change subject: [Log] KUDU-3369 Support compress Kudu log and use size and number of files to manage
......................................................................


Patch Set 24:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/18554/24/src/kudu/util/env.h
File src/kudu/util/env.h:

http://gerrit.cloudera.org:8080/#/c/18554/24/src/kudu/util/env.h@219
PS24, Line 219: farmat
format

Also, add a period in the end of the sentence.


http://gerrit.cloudera.org:8080/#/c/18554/24/src/kudu/util/env.h@222
PS24, Line 222: thime
time


http://gerrit.cloudera.org:8080/#/c/18554/24/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

http://gerrit.cloudera.org:8080/#/c/18554/24/src/kudu/util/env_posix.cc@1868
PS24, Line 1868: because the access time doesn't matter.
If the access time doesn't matter, why not to set access time to be the same as the modified time?  Wouldn't it be more consistent?

Also, this needs to be documented in the header file as well as an additional side-effect of calling this utility function.


http://gerrit.cloudera.org:8080/#/c/18554/24/src/kudu/util/env_util.h
File src/kudu/util/env_util.h:

http://gerrit.cloudera.org:8080/#/c/18554/24/src/kudu/util/env_util.h@99
PS24, Line 99: bool compress_files = false
By adding the 'compress_files' parameter, the semantics of this method changes, and now the name no longer matches the behavior.

Maybe, keep the signature of the original method as is, but also add a new method CompressExcessFilesByPattern()?


http://gerrit.cloudera.org:8080/#/c/18554/24/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:

http://gerrit.cloudera.org:8080/#/c/18554/24/src/kudu/util/env_util.cc@337
PS24, Line 337: DeleteExcessCompressedFilesByPattern
What's so special in the implementation of this function that is attributed to compressed files?  I couldn't find anything so far, but I might be missing something.

Maybe, this should be named DeleteFilesByPatternIfOverSize() or something?


http://gerrit.cloudera.org:8080/#/c/18554/24/src/kudu/util/env_util.cc@353
PS24, Line 353: emplace_back(std::make_pair(matching_file_path, file_size));
What if the map already contains an element for the 'mtime' key?  Isn't this going to be a no-op, so the information about another file with the same mtime is lost?


http://gerrit.cloudera.org:8080/#/c/18554/24/src/kudu/util/env_util.cc@357
PS24, Line 357: all_matching_files_size > max_files_size
Is this working as expected when 'all_matching_files_size' becomes negative given the fact that 'max_files_size' is of unsigned integer type?


http://gerrit.cloudera.org:8080/#/c/18554/24/src/kudu/util/logging.cc
File src/kudu/util/logging.cc:

http://gerrit.cloudera.org:8080/#/c/18554/24/src/kudu/util/logging.cc@80
PS24, Line 80:              
style nit: the indent is off, it should be either aligned to the opening parenthesis or be 4 spaces from the beginning of the line


http://gerrit.cloudera.org:8080/#/c/18554/24/src/kudu/util/logging.cc@428
PS24, Line 428:   
style nit: the indent is off, it should be 4 spaces from the beginning of the line above



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I418743a0d521177bd3ca3343eb2774af2689d38a
Gerrit-Change-Number: 18554
Gerrit-PatchSet: 24
Gerrit-Owner: yejiabao <ye...@huawei.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: yejiabao <ye...@huawei.com>
Gerrit-Comment-Date: Wed, 28 Dec 2022 01:30:44 +0000
Gerrit-HasComments: Yes