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 4: (17 comments) 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: Done 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: Done 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: Done 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 Done 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 Done http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/env_util.cc@57 PS4, Line 57: files > log files Done 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? Done 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 'sta Done 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 Done http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/logging.h@318 PS4, Line 318: keeps > Keeps Done 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 max_compressed_log_files controls the number of all compressed log files, max_log_files controls the number of log files by log level for example? kudu server sets the following parameters max_log_files=1 max_all_compressed_files_size_mb=600 max_compressed_log_files=800 max_log_size=20 According to this setting, there will be only one log file for each level of logs. When the log size reaches 20MB, it will create a new log, and old log file will be compressed. When the number of all compressed files reaches 800 or the size of all compressed files reaches 600MB, delete the oldest compressed files if the kudu server is restarted repeatedly, a large number of small compressed log files will be generated. In this scenario, the threshold of the total size will not be reached, but the threshold of the number of files in the system will be reached. so i think it make sense to keep the similar semantics for compressed files as well http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/logging.cc@74 PS4, Line 74: all > among all Done http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/logging.cc@74 PS4, Line 74: level > levels Done http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/logging.cc@75 PS4, Line 75: , > The change the comma to a period (.)? Done http://gerrit.cloudera.org:8080/#/c/18554/4/src/kudu/util/logging.cc@75 PS4, Line 75: all log files > all compressed log files Done 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 re Done 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 spe 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: 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-Reviewer: yejiabao <[email protected]> Gerrit-Comment-Date: Sat, 04 Jun 2022 12:01:33 +0000 Gerrit-HasComments: Yes
