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

Fix dataDir not deleted if containing comma #871

Merged
merged 6 commits into from
Nov 3, 2020

Conversation

9547
Copy link
Contributor

@9547 9547 commented Oct 29, 2020

What problem does this PR solve?

fix #865

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change
  • Has persistent data change

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Release notes:

Fix the issue when `dataDir` contains `,` can't detect `CountDir()` exceptions and `dirConflictsDetect()` 

@codecov-io
Copy link

codecov-io commented Oct 29, 2020

Codecov Report

Merging #871 into master will decrease coverage by 3.62%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #871      +/-   ##
==========================================
- Coverage   53.04%   49.42%   -3.63%     
==========================================
  Files         263      263              
  Lines       19030    19042      +12     
==========================================
- Hits        10095     9412     -683     
- Misses       7374     8145     +771     
+ Partials     1561     1485      -76     
Flag Coverage Δ
cluster 39.71% <91.66%> (-5.19%) ⬇️
dm 25.24% <0.00%> (-0.03%) ⬇️
integrate 44.21% <91.66%> (-3.65%) ⬇️
playground 22.17% <ø> (ø)
tiup 10.76% <0.00%> (-0.02%) ⬇️
unittest 20.93% <88.88%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/cluster/operation/action.go 52.44% <ø> (+0.93%) ⬆️
pkg/cluster/spec/validate.go 93.03% <91.42%> (-0.50%) ⬇️
components/cluster/command/prune.go 57.14% <100.00%> (+9.52%) ⬆️
components/cluster/command/check.go 6.37% <0.00%> (-72.82%) ⬇️
pkg/cluster/task/limits.go 0.00% <0.00%> (-68.75%) ⬇️
pkg/cluster/task/sysctl.go 0.00% <0.00%> (-66.67%) ⬇️
components/cluster/command/audit.go 27.27% <0.00%> (-54.55%) ⬇️
pkg/cluster/operation/check.go 0.00% <0.00%> (-51.95%) ⬇️
pkg/cluster/task/rmdir.go 0.00% <0.00%> (-50.00%) ⬇️
pkg/cluster/operation/operation.go 34.78% <0.00%> (-43.48%) ⬇️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a295b4...b4b4be2. Read the comment docs.

Copy link
Contributor

@AstroProfundis AstroProfundis left a comment

Choose a reason for hiding this comment

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

LGTM, some tests need to be fixed

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 2, 2020
@9547 9547 force-pushed the fix/count-dir-if-separated branch from be3084b to e032ead Compare November 2, 2020 13:20
@9547 9547 changed the title WIP: Fix count dir if comma separated Fix dataDir not deleted if containing comma Nov 2, 2020
@9547 9547 force-pushed the fix/count-dir-if-separated branch from c3ee191 to 6041116 Compare November 2, 2020 23:45
@9547
Copy link
Contributor Author

9547 commented Nov 3, 2020

LGTM, some tests need to be fixed

Fixed, and PTAL

@lonng
Copy link
Contributor

lonng commented Nov 3, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 3, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@lucklove lucklove merged commit 871ae58 into pingcap:master Nov 3, 2020
@9547 9547 deleted the fix/count-dir-if-separated branch November 3, 2020 14:14
@lucklove lucklove added this to the v1.2.4 milestone Nov 19, 2020
lucklove pushed a commit that referenced this pull request Nov 19, 2020
* typo(cluster): naming

* cluster/spec: countdir should split dir if ',' seplit

* cluster/spec: dirConflict split data_dir

* tests/tiup-cluster: add case of tiflash scale-in/out with multi dir

* cluster/spec: trim dataDir's space before count

Co-authored-by: ti-srebot <66930949+ti-srebot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Destroy Command Does Not Clean TiFlash Data Directories
7 participants