yejiabao 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? Done 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 un Done 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 th Done 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? Done 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 suc Done 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 Done 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 t Done 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 'cons Done 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 Done -- 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: Sun, 16 Oct 2022 07:45:18 +0000 Gerrit-HasComments: Yes
