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

Playground: support setting custom timeout for waiting instances up #968

Merged
merged 10 commits into from
Dec 8, 2020

Conversation

unbyte
Copy link
Contributor

@unbyte unbyte commented Dec 3, 2020

What problem does this PR solve?

#962

Currently tiup playground will wait for tidb up in 60 seconds, and if it fails, tiflash instance will be ignored. However, sometimes tidb instances start normally but only the startup time exceeds the timeout time. So maybe support setting custom timeout for waiting instances up can help.

What is changed and how it works?

  1. add two flag
--db.timeout int           TiDB max wait time in seconds for staring, 0 means no limit (default 60)
--tiflash.timeout int      TiFlash max wait time in seconds for staring, 0 means no limit (default 120)
  1. remove waiting for tikv

  2. check tidb and tiflash status in parallel

image

Check List

Tests

  • Manual test (add detailed scripts or steps below)

Related changes

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

Release notes:

NONE

close #962

@ti-chi-bot ti-chi-bot requested a review from july2993 December 3, 2020 14:48
@ti-chi-bot ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 3, 2020
@codecov-io
Copy link

codecov-io commented Dec 3, 2020

Codecov Report

Merging #968 (bb5dc14) into master (4ac0eaa) will decrease coverage by 4.24%.
The diff coverage is 36.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #968      +/-   ##
==========================================
- Coverage   55.79%   51.54%   -4.25%     
==========================================
  Files         263      263              
  Lines       19541    19768     +227     
==========================================
- Hits        10903    10190     -713     
- Misses       6907     7937    +1030     
+ Partials     1731     1641      -90     
Flag Coverage Δ
cluster 38.20% <ø> (-5.10%) ⬇️
dm 24.31% <ø> (ø)
integrate 46.27% <41.55%> (-3.71%) ⬇️
playground 20.56% <41.55%> (+0.30%) ⬆️
tiup 16.79% <ø> (ø)
unittest 23.04% <0.00%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
components/playground/instance/instance.go 45.45% <ø> (ø)
components/playground/playground.go 30.56% <32.83%> (-10.72%) ⬇️
components/playground/main.go 72.30% <50.00%> (+2.80%) ⬆️
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 28 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 4ac0eaa...bb5dc14. Read the comment docs.

@unbyte unbyte changed the title support setting custom timeout for waiting instances up Playground: support setting custom timeout for waiting instances up Dec 3, 2020
@unbyte
Copy link
Contributor Author

unbyte commented Dec 3, 2020

image

Noticed that even if a process such as tiflash exited with error, the function to check status is still polling (this problem exists in both the master branch and this PR). Maybe need to find a way to notify check functions when the process exits with error?

@july2993 july2993 removed their request for review December 4, 2020 02:33
@AstroProfundis AstroProfundis added the status/need-doc Indicates that we should update document before merge a PR. label Dec 4, 2020
@lucklove
Copy link
Member

lucklove commented Dec 8, 2020

/lgtm

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 8, 2020
@lucklove
Copy link
Member

lucklove commented Dec 8, 2020

/lgtm cancel

@ti-chi-bot ti-chi-bot removed the status/LGT1 Indicates that a PR has LGTM 1. label Dec 8, 2020
@unbyte
Copy link
Contributor Author

unbyte commented Dec 8, 2020

solved

@lucklove
Copy link
Member

lucklove commented Dec 8, 2020

/lgtm

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 8, 2020
@lucklove
Copy link
Member

lucklove commented Dec 8, 2020

/merge

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 8, 2020
@ti-chi-bot
Copy link
Member

Can merge label has been added.

Git tree hash: bb5dc14

@ti-chi-bot ti-chi-bot merged commit 7166440 into pingcap:master Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. status/need-doc Indicates that we should update document before merge a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Playground incorrectly detected that tidb failed to start
5 participants