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 2022/03/02 19:45:15 UTC

[Impala-ASF-CR] IMPALA-11152: Remove dependence on symlink when rotating logs

Riza Suminto has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18286


Change subject: IMPALA-11152: Remove dependence on symlink when rotating logs
......................................................................

IMPALA-11152: Remove dependence on symlink when rotating logs

IMPALA-5256 implement log rotation by following the glog's symlink and
checking the size of the pointed file. While this has been robust most
of the time, there can be a rare situation where the symlink is missing.
Glog itself does not guarantee that the symlink creation will always be
successful. It won't retry symlink creation until the next rotation by
glog. The side effect of this issue is that impala::CheckLogSize() will
spam ERROR log every second for not finding the symlink.

This patch removes the dependence on the glog symlink for this log
rotation. We now directly glob the base file name of the targetted log
kind and pick the latest log path. This patch also makes
impala::CheckLogSize() less chatty by printing an error message for
every FLAGS_logbufsecs (default is 30s).

Testing:
- Add TestLogging::test_excessive_cerr_no_symlink.
- Pass test_breakpad.py::TestLogging in exhaustive exploration.

Change-Id: I30509e98038dbf9ca293144089f6ee92ce186a97
---
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 tests/custom_cluster/test_breakpad.py
5 files changed, 85 insertions(+), 30 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/86/18286/1
-- 
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: newchange
Gerrit-Change-Id: I30509e98038dbf9ca293144089f6ee92ce186a97
Gerrit-Change-Number: 18286
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>

[Impala-ASF-CR] IMPALA-11152: Remove dependence on symlink when rotating logs

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Fang-Yu Rao, Joe McDonnell, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/18286

to look at the new patch set (#2).

Change subject: IMPALA-11152: Remove dependence on symlink when rotating logs
......................................................................

IMPALA-11152: Remove dependence on symlink when rotating logs

IMPALA-5256 implement log rotation by following the glog's symlink and
checking the size of the pointed file. While this has been robust most
of the time, there can be a rare situation where the symlink is missing.
Glog itself does not guarantee that the symlink creation will always be
successful. It won't retry symlink creation until the next rotation by
glog. The side effect of this issue is that impala::CheckLogSize() will
spam ERROR log every second for not finding the symlink.

This patch removes the dependence on the glog symlink for this log
rotation. We now directly glob the base file name of the targetted log
kind and pick the latest log path. This patch also makes
impala::CheckLogSize() less chatty by printing an error message for
every FLAGS_logbufsecs (default is 30s).

Testing:
- Add test_breakpad.py::TestLogging::test_excessive_cerr_no_symlink.
- Pass test_breakpad.py in exhaustive exploration.

Change-Id: I30509e98038dbf9ca293144089f6ee92ce186a97
---
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 tests/custom_cluster/test_breakpad.py
5 files changed, 86 insertions(+), 33 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/86/18286/2
-- 
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: newpatchset
Gerrit-Change-Id: I30509e98038dbf9ca293144089f6ee92ce186a97
Gerrit-Change-Number: 18286
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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-11152: Remove dependence on symlink when rotating logs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/10248/ : 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/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 <ri...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Fri, 04 Mar 2022 02:41:12 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11152: Remove dependence on symlink when rotating logs

Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
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 <ri...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 04 Mar 2022 02:43:41 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11152: Remove dependence on symlink when rotating logs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/10243/ : 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/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: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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, 02 Mar 2022 20:04:47 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11152: Remove dependence on symlink when rotating logs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/10247/ : 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/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: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Fri, 04 Mar 2022 02:28:16 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11152: Remove dependence on symlink when rotating logs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6: Verified+1


-- 
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: 6
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Sat, 05 Mar 2022 03:39:07 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11152: Remove dependence on symlink when rotating logs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/10260/ : 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/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: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 04 Mar 2022 21:43:35 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11152: Remove dependence on symlink when rotating logs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/18286/2/tests/custom_cluster/test_breakpad.py
File tests/custom_cluster/test_breakpad.py:

http://gerrit.cloudera.org:8080/#/c/18286/2/tests/custom_cluster/test_breakpad.py@454
PS2, Line 454: +
flake8: E126 continuation line over-indented for hanging indent



-- 
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: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Fri, 04 Mar 2022 02:09:27 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11152: Remove dependence on symlink when rotating logs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6: Code-Review+2


-- 
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: 6
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 04 Mar 2022 22:37:46 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11152: Remove dependence on symlink when rotating logs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7907/ DRY_RUN=false


-- 
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: 6
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 04 Mar 2022 22:37:47 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11152: Remove dependence on symlink when rotating logs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 4:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/10259/ : Initial code review checks failed. See linked job for details on the failure.


-- 
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: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 04 Mar 2022 21:19:05 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11152: Remove dependence on symlink when rotating logs

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Fang-Yu Rao, Qifan Chen, Joe McDonnell, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/18286

to look at the new patch set (#4).

Change subject: IMPALA-11152: Remove dependence on symlink when rotating logs
......................................................................

IMPALA-11152: Remove dependence on symlink when rotating logs

IMPALA-5256 implements log rotation by following the glog's symlink and
checking the size of the pointed file. While this has been robust most
of the time, there can be a rare situation where the symlink is missing.
Glog itself does not guarantee that the symlink creation will always be
successful. It won't retry symlink creation until the next rotation by
glog. The side effect of this issue is that impala::CheckLogSize() will
spam ERROR log every second for not finding the symlink.

This patch removes the dependence on the glog symlink for this log
rotation. We now directly specify the base file name of the targetted
log kind and pick the latest log path. This patch also makes
impala::CheckLogSize() less chatty by printing an error message for
every FLAGS_logbufsecs (default is 30s).

Testing:
- Add test_breakpad.py::TestLogging::test_excessive_cerr_no_symlink.
- Pass test_breakpad.py in exhaustive exploration.

Change-Id: I30509e98038dbf9ca293144089f6ee92ce186a97
---
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 tests/custom_cluster/test_breakpad.py
5 files changed, 140 insertions(+), 50 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/86/18286/4
-- 
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: newpatchset
Gerrit-Change-Id: I30509e98038dbf9ca293144089f6ee92ce186a97
Gerrit-Change-Number: 18286
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>

[Impala-ASF-CR] IMPALA-11152: Remove dependence on symlink when rotating logs

Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
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 5: Code-Review+2

Looks great!


-- 
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: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 04 Mar 2022 22:25:46 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11152: Remove dependence on symlink when rotating logs

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Fang-Yu Rao, Joe McDonnell, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/18286

to look at the new patch set (#3).

Change subject: IMPALA-11152: Remove dependence on symlink when rotating logs
......................................................................

IMPALA-11152: Remove dependence on symlink when rotating logs

IMPALA-5256 implement log rotation by following the glog's symlink and
checking the size of the pointed file. While this has been robust most
of the time, there can be a rare situation where the symlink is missing.
Glog itself does not guarantee that the symlink creation will always be
successful. It won't retry symlink creation until the next rotation by
glog. The side effect of this issue is that impala::CheckLogSize() will
spam ERROR log every second for not finding the symlink.

This patch removes the dependence on the glog symlink for this log
rotation. We now directly glob the base file name of the targetted log
kind and pick the latest log path. This patch also makes
impala::CheckLogSize() less chatty by printing an error message for
every FLAGS_logbufsecs (default is 30s).

Testing:
- Add test_breakpad.py::TestLogging::test_excessive_cerr_no_symlink.
- Pass test_breakpad.py in exhaustive exploration.

Change-Id: I30509e98038dbf9ca293144089f6ee92ce186a97
---
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 tests/custom_cluster/test_breakpad.py
5 files changed, 86 insertions(+), 33 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/86/18286/3
-- 
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: newpatchset
Gerrit-Change-Id: I30509e98038dbf9ca293144089f6ee92ce186a97
Gerrit-Change-Number: 18286
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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-11152: Remove dependence on symlink when rotating logs

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Fang-Yu Rao, Qifan Chen, Joe McDonnell, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/18286

to look at the new patch set (#5).

Change subject: IMPALA-11152: Remove dependence on symlink when rotating logs
......................................................................

IMPALA-11152: Remove dependence on symlink when rotating logs

IMPALA-5256 implements log rotation by following the glog's symlink and
checking the size of the pointed file. While this has been robust most
of the time, there can be a rare situation where the symlink is missing.
Glog itself does not guarantee that the symlink creation will always be
successful. It won't retry symlink creation until the next rotation by
glog. The side effect of this issue is that impala::CheckLogSize() will
spam ERROR log every second for not finding the symlink.

This patch removes the dependence on the glog symlink for this log
rotation. We now directly specify the base file name of the targetted
log kind and pick the latest log path. This patch also makes
impala::CheckLogSize() less chatty by printing an error message for
every FLAGS_logbufsecs (default is 30s).

Testing:
- Add test_breakpad.py::TestLogging::test_excessive_cerr_no_symlink.
- Pass test_breakpad.py in exhaustive exploration.

Change-Id: I30509e98038dbf9ca293144089f6ee92ce186a97
---
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 tests/custom_cluster/test_breakpad.py
5 files changed, 140 insertions(+), 50 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/86/18286/5
-- 
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: newpatchset
Gerrit-Change-Id: I30509e98038dbf9ca293144089f6ee92ce186a97
Gerrit-Change-Number: 18286
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>

[Impala-ASF-CR] IMPALA-11152: Remove dependence on symlink when rotating logs

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto 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 5:

(6 comments)

Patch set 1 mess up other tests within test_breakpad.py because logs from different impalad are put in the same directory.

Patch set 2 to 5 fix that by including impalad's pid in the glob pattern.
I also add assertion to check that the last line of the old log file mention the path of new log file.

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
Done


http://gerrit.cloudera.org:8080/#/c/18286/1//COMMIT_MSG@18
PS1, Line 18: spec
> nit. specify?
Done


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@113
PS1, Line 113:   msg = 
> Seems we can check two other specific error codes (https://linux.die.net/ma
Done


http://gerrit.cloudera.org:8080/#/c/18286/1/be/src/common/logging.cc@123
PS1, Line 123: }
> nit. may add a comment here: find the largest full file path which is also 
Done


http://gerrit.cloudera.org:8080/#/c/18286/1/be/src/common/logging.cc@270
PS1, Line 270:     status = FileSystemUtil::ApproximateFileSize(log_path, file_size);
> nit. Spell out the log_level here: Failed to check log file size for log le
Done


http://gerrit.cloudera.org:8080/#/c/18286/1/be/src/common/logging.cc@278
PS1, Line 278:                << status.GetDetail();
             :     }
> nit. Just wonder if these two lines can be removed, as we will log the info
Agree, removed.



-- 
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: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 04 Mar 2022 21:46:29 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11152: Remove dependence on symlink when rotating logs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18286 )

Change subject: IMPALA-11152: Remove dependence on symlink when rotating logs
......................................................................

IMPALA-11152: Remove dependence on symlink when rotating logs

IMPALA-5256 implements log rotation by following the glog's symlink and
checking the size of the pointed file. While this has been robust most
of the time, there can be a rare situation where the symlink is missing.
Glog itself does not guarantee that the symlink creation will always be
successful. It won't retry symlink creation until the next rotation by
glog. The side effect of this issue is that impala::CheckLogSize() will
spam ERROR log every second for not finding the symlink.

This patch removes the dependence on the glog symlink for this log
rotation. We now directly specify the base file name of the targetted
log kind and pick the latest log path. This patch also makes
impala::CheckLogSize() less chatty by printing an error message for
every FLAGS_logbufsecs (default is 30s).

Testing:
- Add test_breakpad.py::TestLogging::test_excessive_cerr_no_symlink.
- Pass test_breakpad.py in exhaustive exploration.

Change-Id: I30509e98038dbf9ca293144089f6ee92ce186a97
Reviewed-on: http://gerrit.cloudera.org:8080/18286
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 tests/custom_cluster/test_breakpad.py
5 files changed, 140 insertions(+), 50 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

-- 
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: merged
Gerrit-Change-Id: I30509e98038dbf9ca293144089f6ee92ce186a97
Gerrit-Change-Number: 18286
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>