Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/18286 )
Change subject: IMPALA-11152: Remove dependence on symlink when rotating logs ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/18286/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18286/1//COMMIT_MSG@9 PS1, Line 9: implement nit. implements http://gerrit.cloudera.org:8080/#/c/18286/1//COMMIT_MSG@18 PS1, Line 18: glob nit. specify? http://gerrit.cloudera.org:8080/#/c/18286/1/be/src/common/logging.h File be/src/common/logging.h: http://gerrit.cloudera.org:8080/#/c/18286/1/be/src/common/logging.h@128 PS1, Line 128: individual nit. during each log size check via this call? http://gerrit.cloudera.org:8080/#/c/18286/1/be/src/common/logging.cc File be/src/common/logging.cc: http://gerrit.cloudera.org:8080/#/c/18286/1/be/src/common/logging.cc@a247 PS1, Line 247: : Does this imply that we rotate the ERROR and INFO log files in a pair, even when only one of them is too big? Seems we can just keep use a log file if its size does not exceed the threshold. http://gerrit.cloudera.org:8080/#/c/18286/1/be/src/common/logging.cc@113 PS1, Line 113: } else { Seems we can check two other specific error codes (https://linux.die.net/man/3/glob). GLOB_NOSPACE for running out of memory, GLOB_ABORTED for a read error http://gerrit.cloudera.org:8080/#/c/18286/1/be/src/common/logging.cc@123 PS1, Line 123: fo nit. may add a comment here: find the largest full file path which is also the latest. http://gerrit.cloudera.org:8080/#/c/18286/1/be/src/common/logging.cc@270 PS1, Line 270: if (log_error) LOG(ERROR) << "Failed to check log file size: " << status.GetDetail(); nit. Spell out the log_level here: Failed to check log file size for log level <level>: ... http://gerrit.cloudera.org:8080/#/c/18286/1/be/src/common/logging.cc@278 PS1, Line 278: google::SetLogFilenameExtension(".cut"); : google::SetLogFilenameExtension(""); nit. Just wonder if these two lines can be removed, as we will log the info at line 283 and 284. -- To view, visit http://gerrit.cloudera.org:8080/18286 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I30509e98038dbf9ca293144089f6ee92ce186a97 Gerrit-Change-Number: 18286 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Fri, 04 Mar 2022 02:43:41 +0000 Gerrit-HasComments: Yes
