Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore not found error when rotating log file #4971

Conversation

Projects
None yet
5 participants
@sunxiaoguang
Copy link
Collaborator

commented Jun 25, 2019

Do not return not found error when log file was deleted before rotate.

Signed-off-by: Xiaoguang Sun sunxiaoguang@zhihu.com

What have you changed? (mandatory)

Do not return error when log file is deleted before rotating. Although I doubt if we really need to return any error at here. It is definitely better to be conservative and only skip std::io::ErrorKind::NotFound at this time.

Please explain IN DETAIL what the changes are in this PR and why they are needed:

  • Summarize your change (mandatory)
    TiKV is going to panic during rotating if current log file is deleted. This PR changes log rotating behavior and ignore NotFound error kind.

Please NOTE that:

  • Do not assume reviewers understand the original issue

What are the type of the changes? (mandatory)

  • Misc (other changes)
    NA

How has this PR been tested? (mandatory)

Unit test has been updated to validate the change.

Does this PR affect documentation (docs) or release note? (mandatory)

No

  • If there is a document change, please file a PR in ([docs]
    No
  • If this PR should be mentioned in the release note, please update the [release note]
    No

Does this PR affect tidb-ansible update? (mandatory)

No

If there is a configuration or metrics change, please file a PR in [tidb-ansible]
No

Refer to a related PR or issue link (optional)

NA

Benchmark result if necessary (optional)

NA

Add a few positive/negative examples (optional)

NA

Ignore not found error when rotating log file
Do not return not found error when log file was deleted before rotate.

Signed-off-by: Xiaoguang Sun <sunxiaoguang@zhihu.com>
@sre-bot

This comment has been minimized.

Copy link
Collaborator

commented Jun 25, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@@ -67,7 +67,11 @@ impl RotatingFileLogger {

// Note: renaming files while they're open only works on Linux and macOS.
let new_path = rotation_file_path_with_timestamp(&self.file_path, &Utc::now());
fs::rename(&self.file_path, new_path)?;
match fs::rename(&self.file_path, new_path) {

This comment has been minimized.

Copy link
@siddontang

siddontang Jun 25, 2019

Contributor

it is better to use a failpoint to add a test here.

This comment has been minimized.

Copy link
@sunxiaoguang

sunxiaoguang Jun 25, 2019

Author Collaborator

Sounds like a good idea. Let me put one here.

This comment has been minimized.

Copy link
@sunxiaoguang

sunxiaoguang Jun 28, 2019

Author Collaborator

@siddontang failpoint 'file_log_rename' added.

@overvenus
Copy link
Contributor

left a comment

LGTM, thanks!

breeswish and others added some commits Jun 26, 2019

Add failpoint to log file rotating
Signed-off-by: Xiaoguang Sun <sunxiaoguang@zhihu.com>

@sunxiaoguang sunxiaoguang dismissed stale reviews from overvenus and breeswish via a69b4d1 Jun 28, 2019

@sunxiaoguang

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 28, 2019

Test failed mysteriously at not related part. How to trigger a new run of it please?

@sunxiaoguang

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 28, 2019

Test failed mysteriously at not related part. How to trigger a new run of it please?

Merging from master did the trick.

@breeswish

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

/run-integration-tests

@sunxiaoguang

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 2, 2019

/run-integration-tests

@sunxiaoguang sunxiaoguang merged commit 69eaf88 into tikv:master Jul 2, 2019

5 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details

@sunxiaoguang sunxiaoguang deleted the sunxiaoguang:do_not_panic_when_rotating_deleted_log_file branch Jul 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.