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 4: (22 comments) Thank you for working on this! I did a quick review: overall this looks OK to me, but there are a few high-level questions and some nits. http://gerrit.cloudera.org:8080/#/c/18554/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18554/4//COMMIT_MSG@9 PS4, Line 9: This path support compress the rotated log files how about: This patch adds support for compressing rotated log files. http://gerrit.cloudera.org:8080/#/c/18554/4//COMMIT_MSG@10 PS4, Line 10: use 'FLAGS_max_compressed_log_files' to retain the max number : of compressed log files, how about: Use --max_compressed_log_files to control the maximum number of compressed log files to keep. http://gerrit.cloudera.org:8080/#/c/18554/4//COMMIT_MSG@12 PS4, Line 12: use 'FLAGS_max_all_compressed_files_size_mb' to retain the : max size of all compressed files how about: Use --max_all_compressed_files_size_mb to control the total size of all compressed files under a Kudu server. http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/server/server_base.cc@843 PS4, Line 843: compressd compressed http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/env_util.cc File src/kudu/util/env_util.cc: http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/env_util.cc@33 PS4, Line 33: #include <zlib.h> move this into the section with gflags.h and logging.h http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/env_util.cc@57 PS4, Line 57: reserve Why reserve? Is there something reserved upfront? http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/env_util.cc@57 PS4, Line 57: files log files http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/env_util.cc@58 PS4, Line 58: stable I guess it's too early to declare this stable, isn't it? Usually, the 'stable' status is given to flag that have been there for at least one release and it's already clear that it's not going to change in the future. http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/env_util.cc@322 PS4, Line 322: uint64_t all_matching_files_size = 0; With computations below at line 335, there is a risk of underlow on an unsigned integer: consider using a signed type here instead. http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/env_util.cc@323 PS4, Line 323: nit: an extra space (not needed with contemporary C++ compilers) http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/env_util.cc@334 PS4, Line 334: FLAGS_max_all_compressed_files_size_mb To follow the suite with other functions like this, maybe pass the threshold on the total files size as a parameter for this function? http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/env_util.cc@335 PS4, Line 335: matching_file_mtimes.begin() Maybe, introduce a variable in this scope? http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/env_util.cc@396 PS4, Line 396: string nit: add 'const' since this variable is not changing in this scope http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/logging.h File src/kudu/util/logging.h: http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/logging.h@316 PS4, Line 316: // Deletes excess compressed log files nit: add a period (.) in the end of the sentence http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/logging.h@318 PS4, Line 318: keeps Keeps http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/logging.cc File src/kudu/util/logging.cc: http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/logging.cc@73 PS4, Line 73: max_compressed_log_files BTW, max_log_files are per severity level. Does it make sense to keep the similar semantics for compressed files as well, given there is also a threshold on the total size of all compressed files? What do you think? http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/logging.cc@74 PS4, Line 74: level levels http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/logging.cc@74 PS4, Line 74: all among all http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/logging.cc@75 PS4, Line 75: , The change the comma to a period (.)? http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/logging.cc@75 PS4, Line 75: all log files all compressed log files http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/logging.cc@77 PS4, Line 77: stable Usually, a flag gets the 'stable' tag after a while (maybe, at least one release). I guess at this point it's not yet clear whether it's going to be changes in there in the nearest future. http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/logging.cc@411 PS4, Line 411: Ignore bad input You could make the flag to be uint32_t to avoid this. 0 can be still a special value. -- 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: 4 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-Comment-Date: Wed, 01 Jun 2022 18:44:59 +0000 Gerrit-HasComments: Yes
