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 data_dir of tiflash when import cluster #612

Merged
merged 3 commits into from
Jul 21, 2020
Merged

Fix data_dir of tiflash when import cluster #612

merged 3 commits into from
Jul 21, 2020

Conversation

july2993
Copy link
Contributor

What problem does this PR solve?

fix #611
import command will only get data_dir from inventory.ini and start
script of tiflash and the start script contains the only config file
flag, so it can not get the truly use path and use the default value.

What is changed and how it works?

parse the config file of tiflash to get path and tmp_path and set it in DataDir and TmpDir in spec.

test

  • unit-test
  • manual-test
    • test using import command and check the spec.

fix #611
import command will only get data_dir from inventory.ini and start
script of tiflash and the start script contains the only config file
flag, so it can not get the truly use path and use the default value.
@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2020

Codecov Report

Merging #612 into master will increase coverage by 0.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #612      +/-   ##
==========================================
+ Coverage   51.20%   51.32%   +0.12%     
==========================================
  Files         225      225              
  Lines       16573    16606      +33     
==========================================
+ Hits         8486     8523      +37     
+ Misses       6901     6898       -3     
+ Partials     1186     1185       -1     
Flag Coverage Δ
#coverage 51.32% <ø> (+0.12%) ⬆️

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

Impacted Files Coverage Δ
...m/pingcap/tiup/components/playground/playground.go 30.57% <0.00%> (+0.99%) ⬆️
...c/github.com/pingcap/tiup/pkg/cluster/api/pdapi.go 60.24% <0.00%> (+2.48%) ⬆️
...ithub.com/pingcap/tiup/pkg/cluster/ansible/dirs.go 4.00% <0.00%> (+4.00%) ⬆️
.../pingcap/tiup/components/playground/utils/utils.go 31.91% <0.00%> (+31.91%) ⬆️

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 10a8d77...c20b64f. Read the comment docs.

@@ -123,6 +125,21 @@ func parseDirs(user string, ins spec.InstanceSpec, sshTimeout int64) (spec.Insta
newIns.DeployDir = strings.Trim(strings.Split(line, " ")[1], "\"")
continue
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to add some tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

forgot to commit the file: 6dd6f95
but it only covers about parsing the file and get info

Copy link
Contributor

@lonng lonng 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 Jul 21, 2020
@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jul 21, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jul 21, 2020
err := parseTiflashConfigFromFileData(spec, data)
c.Assert(err, check.IsNil)

c.Assert(spec.DataDir, check.Equals, "/data1/test-cluster/leiysky-ansible-test-deploy/tiflash/data/db")
Copy link
Contributor

Choose a reason for hiding this comment

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

TiFlash supports multiple comma seperated paths like /path/1,/path/2, we should also add cases for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here we just need to make sure DataDir is the same as path in tiflash.toml, so I don't think we need to add suck case here.

@lonng lonng merged commit 9d5f96e into master Jul 21, 2020
@lonng lonng deleted the flash_dir branch July 21, 2020 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wrong data-dir of tiflash after import from ansible
5 participants