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.

Sign up for GitHub

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

Jump to bottom

util: create the tmpdir if the directory is removed by mistake. #18970

Merged
merged 16 commits into from
Aug 18, 2020
Merged

util: create the tmpdir if the directory is removed by mistake. #18970

merged 16 commits into from
Aug 18, 2020

Conversation

wshwsh12
Copy link
Contributor

@wshwsh12 wshwsh12 commented Aug 4, 2020 •
edited
Loading

What problem does this PR solve?

Issue Number:
close #18960
close #18055

Problem Summary:

  1. If the temp directory is removed, all SQLs that trigger spill action will get unexpected error open: ...... no such file or directory.
  2. If remove some files failed, log the error and continue to remove other temp files.
  3. If there is a temp file removed failed when calling Close, other temp files can't remove successfully.

What is changed and how it works?

Proposal: xxx

What's Changed:

  1. Before creating temp files and writing them, check the temp directory is valid. If the directory invalid, create the directory before spilling.
  2. Log and ignore the error when calling Close().

How it Works:

Related changes

  • Need to cherry-pick to the release branch 4.0

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
    Case 1:
  1. Start a tidb instance.
  2. Remove the temp directory.
  3. Run a SQL that spills some data to disk.
  4. SQL runs successfully, and the temp directory is created again.

Case 2:

  1. Start a tidb instance.
  2. Run a SQL that spills some data to disk.
  3. Remove some temp files when running.
  4. SQL runs failed, and the other temp files are removed successfully.

Side effects

Release note

  • Fix unexpected error when spilling disk.

@wshwsh12 wshwsh12 requested a review from a team as a code owner August 4, 2020 08:43
@wshwsh12 wshwsh12 requested review from qw4990 and XuHuaiyu and removed request for a team August 4, 2020 08:43
@wshwsh12 wshwsh12 added sig/execution SIG execution needs-cherry-pick-4.0 type/bugfix This PR fixes a bug. labels Aug 4, 2020
@codecov
Copy link

codecov bot commented Aug 4, 2020 •
edited
Loading

Codecov Report

Merging #18970 into master will decrease coverage by 0.3980%.
The diff coverage is 50.8771%.

@@               Coverage Diff                @@
##             master     #18970        +/-   ##
================================================
- Coverage   79.4647%   79.0666%   -0.3981%     
================================================
  Files           549        550         +1     
  Lines        151734     148801      -2933     
================================================
- Hits         120575     117652      -2923     
- Misses        21632      21646        +14     
+ Partials       9527       9503        -24

@SunRunAway SunRunAway mentioned this pull request Aug 14, 2020
Merged
SunRunAway
SunRunAway reviewed Aug 14, 2020
View reviewed changes
util/disk/tempDir.go Outdated Show resolved Hide resolved
util/disk/tempDir_test.go Outdated Show resolved Hide resolved
SunRunAway
SunRunAway reviewed Aug 17, 2020
View reviewed changes
util/disk/tempDir.go Outdated Show resolved Hide resolved
util/disk/tempDir_test.go Show resolved Hide resolved
SunRunAway
SunRunAway reviewed Aug 17, 2020
View reviewed changes
Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 17, 2020
@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 18, 2020
@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 18916

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@wshwsh12 merge failed.

@wshwsh12
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 18732
  • 18846
  • 19237
  • 19237
  • 19237
  • 19202

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@wshwsh12 merge failed.

@wshwsh12
Copy link
Contributor Author

/run-unit-test
/run-integration-copr-test

@wshwsh12
Copy link
Contributor Author

/run-unit-test

@wshwsh12
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@wshwsh12 merge failed.

@wshwsh12
Copy link
Contributor Author

/run-common-test
/run-sqllogic-test

@wshwsh12
Copy link
Contributor Author

/run-integration-ddl-test

@wshwsh12
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@wshwsh12 merge failed.

@wshwsh12
Copy link
Contributor Author

/run-integration-copr-test
/run-unit-test

@wshwsh12 wshwsh12 merged commit 92513a2 into pingcap:master Aug 18, 2020
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Aug 18, 2020
@wshwsh12 @ti-srebot
cherry pick pingcap#18970 to release-4.0
ce71fc7 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Aug 18, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #19272

jebter pushed a commit that referenced this pull request Aug 19, 2020
@ti-srebot @wshwsh12
util: create the tmpdir if the directory is removed by mistake. (#18970…
a113adf 
…) (#19272)

* cherry pick #18970 to release-4.0

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

* fix cherry-pick

* remove useless line

Co-authored-by: Shenghui Wu <793703860@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SunRunAway SunRunAway SunRunAway left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

@XuHuaiyu XuHuaiyu XuHuaiyu approved these changes

@qw4990 qw4990 Awaiting requested review from qw4990

Assignees
No one assigned
Labels
component/config component/executor sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unexpected error no such file executing sql. Can't remove Tmpfile when RowContainer.Close throw some error.
4 participants
@wshwsh12 @XuHuaiyu @ti-srebot @SunRunAway