Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/17997 )
Change subject: IMPALA-5256: Force log rolling when max_log_size exceeded ...................................................................... Patch Set 1: (7 comments) I'm glad to see this getting fixed, the old behavior is quite weird. http://gerrit.cloudera.org:8080/#/c/17997/1/be/src/common/logging.cc File be/src/common/logging.cc: http://gerrit.cloudera.org:8080/#/c/17997/1/be/src/common/logging.cc@156 PS1, Line 156: void impala::ResolveLogSymlink(const string& symlink_path, string& canonical_path) { : bool is_symbolic_link; : string resolved_path; : Status status = : FileSystemUtil::IsSymbolicLink(symlink_path, &is_symbolic_link, &resolved_path); : canonical_path = (status.ok() && is_symbolic_link) ? resolved_path : symlink_path; : } Nit: One question is who is responsible for handling errors and who can most effectively handle the error. I'm thinking it makes more sense to return Status here and have callers decide what do themselves. http://gerrit.cloudera.org:8080/#/c/17997/1/be/src/common/logging.cc@164 PS1, Line 164: void impala::AttachStdoutStderrLocked() { Nit: I go back and forth about whether this should return Status or not. It might make this code cleaner to be able to return an error. For example, if ResolveLogSymlink() is changed to return status and fails, this function could simply RETURN_IF_ERROR and not have to deal with it here. If it returned not-ok status, my guess is that callers would just log and try again later. http://gerrit.cloudera.org:8080/#/c/17997/1/be/src/common/logging.cc@202 PS1, Line 202: bool impala::CheckLogSize(bool force_roll) { Nit: There are two use cases for this function. One is checking the file size (force_roll = false). The other is checking the file size and rotating if needed (force_roll = true). What do you think about making this two functions? bool impala::CheckLogSize() - return true if any file is too big void impala::ForceRotateLog() - do the rotation http://gerrit.cloudera.org:8080/#/c/17997/1/be/src/common/logging.cc@211 PS1, Line 211: max_log_file_exceeded |= status.ok() && (file_size >> 20) >= FLAGS_max_log_size; Can you add a comment saying that max_log_size is measured in megabytes (and that's why we convert to megabytes by >> 20)? http://gerrit.cloudera.org:8080/#/c/17997/1/be/src/util/filesystem-util.h File be/src/util/filesystem-util.h: http://gerrit.cloudera.org:8080/#/c/17997/1/be/src/util/filesystem-util.h@111 PS1, Line 111: static Status ApproximateFileSize(const std::string& path, uintmax_t& file_size); Please add a comment for this function http://gerrit.cloudera.org:8080/#/c/17997/1/be/src/util/filesystem-util.cc File be/src/util/filesystem-util.cc: http://gerrit.cloudera.org:8080/#/c/17997/1/be/src/util/filesystem-util.cc@458 PS1, Line 458: file_size = filesystem::file_size(path); >From what I understand, this function could throw an exception. boost provides >an equivalent function that has a system::error_code argument. It would be >better to use that one and handle the error directly. We check that the path exists, but better safe than sorry. http://gerrit.cloudera.org:8080/#/c/17997/1/tests/custom_cluster/test_breakpad.py File tests/custom_cluster/test_breakpad.py: http://gerrit.cloudera.org:8080/#/c/17997/1/tests/custom_cluster/test_breakpad.py@427 PS1, Line 427: start = time.time() : expected_impalad_error_logs = test_max_log_files : while (time.time() - start < 20): : time.sleep(1) : assert self.count_error_logs('impalad') == expected_impalad_error_logs Two thoughts: - Do we want to go a bit longer to let it rotate a couple more times? Maybe 40 seconds? - Should we have a check for the log file sizes as we go? Not that strict, but more of a sanity check -- To view, visit http://gerrit.cloudera.org:8080/17997 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1b94727180354fe69989ebf3cd1a8f8cda1cf0c3 Gerrit-Change-Number: 17997 Gerrit-PatchSet: 1 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Comment-Date: Fri, 05 Nov 2021 00:17:51 +0000 Gerrit-HasComments: Yes
