Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/17997 )
Change subject: IMPALA-5256: Force log rotation when max_log_size exceeded ...................................................................... Patch Set 2: (9 comments) 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: FLAGS_stderrthreshold = google::FATAL + 1; : : if (RedirectStdoutStderr()) { : // We will be redirecting stdout/stderr to INFO/LOG so override any glog settings : // that log to stdout/stderr... : FLAGS_logtostderr = false; : > Nit: One question is who is responsible for handling errors and who can mos Done. Also remove the declaration from logging.h to avoid the need to include "common/status.h" there. http://gerrit.cloudera.org:8080/#/c/17997/1/be/src/common/logging.cc@164 PS1, Line 164: > Nit: I go back and forth about whether this should return Status or not. It Done http://gerrit.cloudera.org:8080/#/c/17997/1/be/src/common/logging.cc@202 PS1, Line 202: lock_guard<mutex> logging_lock(logging_mut > Nit: There are two use cases for this function. One is checking the file si Done http://gerrit.cloudera.org:8080/#/c/17997/1/be/src/common/logging.cc@211 PS1, Line 211: int log_to_check[2] = {google::INFO, google::ERROR}; > Can you add a comment saying that max_log_size is measured in megabytes (an Done 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: /// Return the approximate file size of 'path' into output argument 'file_size', > Please add a comment for this function Done 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@416 PS1, Line 416: o > flake8: E261 at least two spaces before inline comment Done http://gerrit.cloudera.org:8080/#/c/17997/1/tests/custom_cluster/test_breakpad.py@417 PS1, Line 417: > flake8: E261 at least two spaces before inline comment Done http://gerrit.cloudera.org:8080/#/c/17997/1/tests/custom_cluster/test_breakpad.py@427 PS1, Line 427: test_max_log_files = 2 : test_max_log_size = 1 # 1 MB : test_error_msg = ('123456789abcde_' * 64) # 1 KB error message : test_debug_actions = 'LOG_MAINTENANCE_STDERR:[email protected]@' + test_error_msg : os.chmod(self.tmp_dir, 0744) > Two thoughts: Done. Increased the wait time to 40s and sanity check the log size and count every second. http://gerrit.cloudera.org:8080/#/c/17997/1/tests/custom_cluster/test_breakpad.py@431 PS1, Line 431: > flake8: W292 no newline at end of file Done -- 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: 2 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Mon, 08 Nov 2021 20:53:42 +0000 Gerrit-HasComments: Yes
