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 19: (9 comments) http://gerrit.cloudera.org:8080/#/c/18554/19/src/kudu/util/env_util.h File src/kudu/util/env_util.h: http://gerrit.cloudera.org:8080/#/c/18554/19/src/kudu/util/env_util.h@101 PS19, Line 101: max_all_compressed_files_size What's "max_all_compressed_files_size" in this context? http://gerrit.cloudera.org:8080/#/c/18554/19/src/kudu/util/env_util.h@104 PS19, Line 104: int Use size_t for the size (or ssize_t/int64_t if it's signed). Also, what unit is used for the size? http://gerrit.cloudera.org:8080/#/c/18554/19/src/kudu/util/env_util.h@129 PS19, Line 129: Compress the given path nit: it's not possible to compress a path, but rather compress a file at the specified path http://gerrit.cloudera.org:8080/#/c/18554/19/src/kudu/util/env_util.cc File src/kudu/util/env_util.cc: http://gerrit.cloudera.org:8080/#/c/18554/19/src/kudu/util/env_util.cc@353 PS19, Line 353: matching_file_mtimes.insert({mtime, {matching_file_path, file_size}}); What if mtime is the same for multiple files? http://gerrit.cloudera.org:8080/#/c/18554/19/src/kudu/util/logging.cc File src/kudu/util/logging.cc: http://gerrit.cloudera.org:8080/#/c/18554/19/src/kudu/util/logging.cc@80 PS19, Line 80: Maximum size of all compressed log files retained (in MB). Is 0 a special value here? If so, then please describe the behavior in such a case. Is it possible to have negative value for this flag with some meaningful behavior? If not, maybe change the type of the flag to uint32? http://gerrit.cloudera.org:8080/#/c/18554/19/src/kudu/util/logging.cc@81 PS19, Line 81: TAG_FLAG(max_all_compressed_log_files_size_mb, stable); : TAG_FLAG(max_all_compressed_log_files_size_mb, experimental); A flag cannot be both 'stable' and 'experimental'. I guess you meant it to be just 'experimental'? http://gerrit.cloudera.org:8080/#/c/18554/19/src/kudu/util/logging.cc@414 PS19, Line 414: int32_t max_compressed_log_files = FLAGS_max_compressed_log_files; nit: add 'const' since this variable isn't changing in the whole scope of this method http://gerrit.cloudera.org:8080/#/c/18554/19/src/kudu/util/logging.cc@427 PS19, Line 427: int Use either int64 or size_t to avoid integer overflow here. Also, add 'const' since this isn't going to change for this scope. http://gerrit.cloudera.org:8080/#/c/18554/19/src/kudu/util/logging.cc@431 PS19, Line 431: RETURN_NOT_OK(env_util::DeleteExcessFilesByPattern(env, pattern, max_compressed_log_files)); : return Status::OK(); nit: shorten these lines to return env_util::DeleteExcessFilesByPattern(...); -- 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: 19 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: yejiabao <[email protected]> Gerrit-Comment-Date: Tue, 11 Oct 2022 03:06:06 +0000 Gerrit-HasComments: Yes
