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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: yejiabao <[email protected]> Gerrit-Comment-Date: Wed, 28 Dec 2022 01:30:44 +0000 Gerrit-HasComments: Yes
