Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

checker: increase total timeout to 30m; set readTimeout for DB check operation to 30s #315

Merged
merged 9 commits into from
Oct 21, 2019

Conversation

csuzhangxc
Copy link
Member

What problem does this PR solve?

If a mass of tables in one task, the check operation may not be complete in 1 minute.

What is changed and how it works?

  1. increase the total check timeout to 30 minutes
  2. set readTimeout for DB operation to 30s

Check List

Tests

  • Unit test
  • Integration test

Related changes

  • Need to cherry-pick to the release branch

@csuzhangxc csuzhangxc added priority/normal Minor change, requires approval from ≥1 primary reviewer status/PTAL This PR is ready for review. Add this label back after committing new changes type/bug-fix Bug fix priority/release-blocker This PR blocks a release. Please review it ASAP. needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 labels Oct 16, 2019
@csuzhangxc
Copy link
Member Author

/run-all-tests

1 similar comment
@csuzhangxc
Copy link
Member Author

/run-all-tests

@codecov
Copy link

codecov bot commented Oct 16, 2019

Codecov Report

Merging #315 into master will increase coverage by 0.438%.
The diff coverage is 0%.

@@               Coverage Diff               @@
##             master       #315       +/-   ##
===============================================
+ Coverage   59.5074%   59.9455%   +0.438%     
===============================================
  Files           135        135               
  Lines         15267      15047      -220     
===============================================
- Hits           9085       9020       -65     
+ Misses         5301       5151      -150     
+ Partials        881        876        -5

@csuzhangxc
Copy link
Member Author

@WangXiangUSTC @lichunzhu PTAL

Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

LGTM

@lichunzhu lichunzhu added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Oct 18, 2019
Copy link
Contributor

@WangXiangUSTC WangXiangUSTC left a comment

Choose a reason for hiding this comment

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

rest LGTM

@@ -125,7 +126,9 @@ func (c *Checker) Init() (err error) {
User: instance.cfg.From.User,
Password: instance.cfg.From.Password,
}
instance.sourceDB, err = dbutil.OpenDB(*instance.sourceDBinfo)
dbCfg := instance.cfg.From
dbCfg.RawDBCfg = config.DefaultRawDBConfig().SetReadTimeout("30s")
Copy link
Contributor

Choose a reason for hiding this comment

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

how about define a const value for 30s and 30*time.Minute?

Copy link
Contributor

Choose a reason for hiding this comment

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

instance is a pointer, we need make sure it will not do influence to others if we SetReadTimeout here

Copy link
Member Author

Choose a reason for hiding this comment

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

instance only used for checking and I want all operation in checking affected by this readTime.

BTW, instance.cfg.From is a value.

Copy link
Member Author

Choose a reason for hiding this comment

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

how about define a const value for 30s and 30*time.Minute?

addressed in c72263d.

Copy link
Contributor

Choose a reason for hiding this comment

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

30s not define as a value

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 done in 1bc7a3f.

Copy link
Contributor

@WangXiangUSTC WangXiangUSTC left a comment

Choose a reason for hiding this comment

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

LGTM

@WangXiangUSTC WangXiangUSTC added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Oct 21, 2019
@csuzhangxc csuzhangxc merged commit 938ccd4 into pingcap:master Oct 21, 2019
@csuzhangxc csuzhangxc deleted the check-context-timeout branch October 21, 2019 03:37
@sre-bot
Copy link

sre-bot commented Oct 21, 2019

cherry pick to release-1.0 in PR #327

@sre-bot sre-bot added the already-cherry-pick-1.0 The related PR is already cherry-picked to release-1.0. Add this label once the PR is cherry-picked label Oct 21, 2019
@sre-bot sre-bot removed the needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 label Oct 21, 2019
csuzhangxc pushed a commit that referenced this pull request Oct 21, 2019
lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
already-cherry-pick-1.0 The related PR is already cherry-picked to release-1.0. Add this label once the PR is cherry-picked priority/normal Minor change, requires approval from ≥1 primary reviewer priority/release-blocker This PR blocks a release. Please review it ASAP. status/LGT2 Two reviewers already commented LGTM, ready for merge type/bug-fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants