You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Riza Suminto (Code Review)" <ge...@cloudera.org> on 2021/11/03 19:56:51 UTC
[Impala-ASF-CR] IMPALA-5256: Force log rolling when max log size exceeded
Riza Suminto has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17997
Change subject: IMPALA-5256: Force log rolling when max_log_size exceeded
......................................................................
IMPALA-5256: Force log rolling when max_log_size exceeded
Impala backends allow STDOUT/STDERR redirection into INFO/ERROR log
respectively though redirect_stdout_stderr startup flag. It does so by
redirecting STDOUT/STDERR stream to write into the log file symlink
created by glog. There are two problems with this approach:
1. When glog rolls the log and starts writing to a new log file, it
updates the symlink. But Impala is not aware that the file pointed
by the symlink has changed. So cout/cerr write still goes to the
oldest log file.
2. When there is a lot of write activity to cout/cerr, the log file can
grow big. However, glog is not aware of STDOUT/STDERR activity. It
only counts the message bytes that were written to glog (LOG(INFO),
LOG(ERROR)). Thus, it only uses its internal bytes count when
deciding to roll the logs.
This commit addresses the issue by monitoring the log file size every
second. If Impala sees that the log file has exceeded max_log_size, it
will call google::FlushLogFiles(), ahead of logbufsecs. If the log file
stays big after the flush, we will force glog to roll the log. Since
there is no direct way to force glog to roll, we do this by changing the
log extension to random extension through
google::SetLogFilenameExtension(), and immediately return them to
extensionless (empty string extension).
We also check periodically whether the log file symlink has pointed to a
new file. If it has changed, we reattach the STDOUT/STDERR to the new
log file.
Testing:
- Pass the core test.
- Add custom cluster test TestLogging::test_excessive_cerr.
Change-Id: I1b94727180354fe69989ebf3cd1a8f8cda1cf0c3
---
M be/src/common/init.cc
M be/src/common/logging.cc
M be/src/common/logging.h
M be/src/util/filesystem-util.cc
M be/src/util/filesystem-util.h
M tests/custom_cluster/test_breakpad.py
6 files changed, 198 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/17997/1
--
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: newchange
Gerrit-Change-Id: I1b94727180354fe69989ebf3cd1a8f8cda1cf0c3
Gerrit-Change-Number: 17997
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-5256: Force log rotation when max log size exceeded
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7617/ DRY_RUN=false
--
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: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Nov 2021 04:36:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5256: Force log rotation when max log size exceeded
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7608/ DRY_RUN=true
--
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 <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Nov 2021 00:36:39 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5256: Force log rotation when max log size exceeded
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17997 )
Change subject: IMPALA-5256: Force log rotation when max_log_size exceeded
......................................................................
IMPALA-5256: Force log rotation when max_log_size exceeded
Impala daemons allow STDOUT/STDERR redirection into INFO/ERROR log
respectively through redirect_stdout_stderr startup flag. If
redirect_stdout_stderr is true, daemons redirect STDOUT/STDERR stream to
write into the log file symlink created by glog. There are two problems
with this approach:
1. Glog updates the symlink to point to the new log file when it does
log rotation. However, Impala is not aware that the symlink point to
a different file. So cout/cerr write still goes to the oldest log
file.
2. When there is a lot of write activity to cout/cerr, the log file can
grow big. However, glog is not aware of STDOUT/STDERR activity. It
only counts the message bytes written to glog (LOG(INFO),
LOG(ERROR)). Thus, it only uses its internal bytes count when
deciding to rotate the logs.
This commit addresses the issue by monitoring the log file size every
second. If Impala sees that the log file has exceeded max_log_size, it
will call google::FlushLogFiles(), ahead of logbufsecs. If the log file
stays big after the flush, we will force the glog to rotate the log.
Since there is no direct way to force glog to rotate, we do this by
changing the log extension to random extension through
google::SetLogFilenameExtension(), and immediately return them to
extensionless (empty string extension).
We also check periodically whether the log file symlink has pointed to a
new file. If it has changed, we reattach the STDOUT/STDERR stream to the
new log file.
Testing:
- Pass the core test.
- Add new exhaustive test TestLogging::test_excessive_cerr.
Change-Id: I1b94727180354fe69989ebf3cd1a8f8cda1cf0c3
Reviewed-on: http://gerrit.cloudera.org:8080/17997
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/common/init.cc
M be/src/common/logging.cc
M be/src/common/logging.h
M be/src/util/filesystem-util.cc
M be/src/util/filesystem-util.h
M tests/custom_cluster/test_breakpad.py
6 files changed, 254 insertions(+), 35 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
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: merged
Gerrit-Change-Id: I1b94727180354fe69989ebf3cd1a8f8cda1cf0c3
Gerrit-Change-Number: 17997
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-5256: Force log rotation when max log size exceeded
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/7608/
--
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 <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Nov 2021 07:28:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5256: Force log rotation when max log size exceeded
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17997
to look at the new patch set (#2).
Change subject: IMPALA-5256: Force log rotation when max_log_size exceeded
......................................................................
IMPALA-5256: Force log rotation when max_log_size exceeded
Impala daemons allow STDOUT/STDERR redirection into INFO/ERROR log
respectively through redirect_stdout_stderr startup flag. If
redirect_stdout_stderr is true, daemons redirect STDOUT/STDERR stream to
write into the log file symlink created by glog. There are two problems
with this approach:
1. Glog updates the symlink to point to the new log file when it does
log rotation. However, Impala is not aware that the symlink point to
a different file. So cout/cerr write still goes to the oldest log
file.
2. When there is a lot of write activity to cout/cerr, the log file can
grow big. However, glog is not aware of STDOUT/STDERR activity. It
only counts the message bytes written to glog (LOG(INFO),
LOG(ERROR)). Thus, it only uses its internal bytes count when
deciding to rotate the logs.
This commit addresses the issue by monitoring the log file size every
second. If Impala sees that the log file has exceeded max_log_size, it
will call google::FlushLogFiles(), ahead of logbufsecs. If the log file
stays big after the flush, we will force the glog to rotate the log.
Since there is no direct way to force glog to rotate, we do this by
changing the log extension to random extension through
google::SetLogFilenameExtension(), and immediately return them to
extensionless (empty string extension).
We also check periodically whether the log file symlink has pointed to a
new file. If it has changed, we reattach the STDOUT/STDERR stream to the
new log file.
Testing:
- Pass the core test.
- Add new exhaustive test TestLogging::test_excessive_cerr.
Change-Id: I1b94727180354fe69989ebf3cd1a8f8cda1cf0c3
---
M be/src/common/init.cc
M be/src/common/logging.cc
M be/src/common/logging.h
M be/src/util/filesystem-util.cc
M be/src/util/filesystem-util.h
M tests/custom_cluster/test_breakpad.py
6 files changed, 254 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/17997/2
--
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: newpatchset
Gerrit-Change-Id: I1b94727180354fe69989ebf3cd1a8f8cda1cf0c3
Gerrit-Change-Number: 17997
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
[Impala-ASF-CR] IMPALA-5256: Force log rotation when max log size exceeded
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9735/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Nov 2021 21:14:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5256: Force log rotation when max log size exceeded
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell 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 3: Code-Review+2
Carrying +2
--
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: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Nov 2021 16:46:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5256: Force log rotation when max log size exceeded
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
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 4:
> Patch Set 4: Verified-1
>
> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/7617/
This one hit flaky test described at IMPALA-10999.
--
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: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Nov 2021 21:07:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5256: Force log rotation when max log size exceeded
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
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:FAIL@1.0@' + 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 <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Nov 2021 20:53:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5256: Force log rotation when max log size exceeded
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7613/ DRY_RUN=true
--
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: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Nov 2021 16:56:25 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5256: Force log rolling when max log size exceeded
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9718/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Nov 2021 20:18:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5256: Force log rotation when max log size exceeded
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 4: Verified+1
--
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: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 11 Nov 2021 23:43:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5256: Force log rotation when max log size exceeded
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
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 3:
> Patch Set 3: Verified-1
>
> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/7613/
This GVO failed on TestReusePartitionMetadata.test_reuse_partition_meta, flaky test reported by IMPALA-10886.
--
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: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Nov 2021 02:26:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5256: Force log rolling when max log size exceeded
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
(3 comments)
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:
flake8: E261 at least two spaces before inline comment
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
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
--
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 <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Nov 2021 19:57:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5256: Force log rolling when max log size exceeded
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Comment-Date: Fri, 05 Nov 2021 00:17:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5256: Force log rotation when max log size exceeded
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7625/ DRY_RUN=true
--
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: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 11 Nov 2021 17:26:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5256: Force log rotation when max log size exceeded
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell 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: Code-Review+2
Thanks, this looks good to me
--
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 <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Nov 2021 22:24:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5256: Force log rotation when max log size exceeded
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 3: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/7613/
--
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: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Nov 2021 23:07:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5256: Force log rotation when max log size exceeded
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 4: Code-Review+2
--
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: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Nov 2021 04:36:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5256: Force log rotation when max log size exceeded
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 4: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/7617/
--
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: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Nov 2021 10:57:13 +0000
Gerrit-HasComments: No