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

add failpoint test for db operation #206

Merged
merged 27 commits into from
Aug 1, 2019
Merged

add failpoint test for db operation #206

merged 27 commits into from
Aug 1, 2019

Conversation

WangXiangUSTC
Copy link
Contributor

@WangXiangUSTC WangXiangUSTC commented Jul 18, 2019

What problem does this PR solve?

add some failpoint about db operation, and add integration test
issue: https://internal.pingcap.net/jira/browse/TOOL-1394

What is changed and how it works?

  1. add some fail point about db operation
  2. use these fail point to add integration test
  • initial unit failed, and find a bug which will lead to panic on dm-worker and dm-master
  • start task failed
  • relay log failed

Check List

Tests

  • Integration test

@WangXiangUSTC WangXiangUSTC added the status/WIP This PR is still work in progress label Jul 18, 2019
@codecov
Copy link

codecov bot commented Jul 19, 2019

Codecov Report

Merging #206 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master       #206   +/-   ##
===========================================
  Coverage   58.8389%   58.8389%           
===========================================
  Files           123        123           
  Lines         14487      14487           
===========================================
  Hits           8524       8524           
  Misses         5098       5098           
  Partials        865        865

@amyangfei amyangfei added priority/normal Minor change, requires approval from ≥1 primary reviewer type/qa relate to quality assurance labels Jul 19, 2019
@WangXiangUSTC WangXiangUSTC added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/WIP This PR is still work in progress labels Jul 24, 2019
@WangXiangUSTC WangXiangUSTC marked this pull request as ready for review July 24, 2019 05:29
Copy link
Contributor

@amyangfei amyangfei left a comment

Choose a reason for hiding this comment

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

Note the integration test runs in parallel, and new added test case is not listed in CI, so remember to add the TEST_NAME of new added test case to file others_integration.txt in a newline.

Newly added integration tests are not tested now

@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

1 similar comment
@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

dm/worker/subtask.go Outdated Show resolved Hide resolved
@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

@WangXiangUSTC
Copy link
Contributor Author

@amyangfei have already add new test in others_integration.txt

@WangXiangUSTC WangXiangUSTC added the priority/important Major change, requires approval from ≥2 primary reviewers label Jul 26, 2019
loader/db.go Outdated

failpoint.Inject("LoadExecCreateTableFailed", func() {
if i == 0 && len(sqls) == 1 && strings.Contains(sqls[0], "CREATE TABLE") {
err = domain.ErrInfoSchemaChanged
Copy link
Collaborator

Choose a reason for hiding this comment

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

why only inject ErrInfoSchemaChanged error here? can we support to inject custom error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

use error code, 3b35795

@@ -59,7 +59,8 @@ function run() {
check_row_count 1
check_row_count 2

export GO_FAILPOINTS=''
# only failed at the first time, will retry later and success
export GO_FAILPOINTS='github.com/pingcap/dm/loader/LoadExecCreateTableFailed=return(true)'
Copy link
Collaborator

Choose a reason for hiding this comment

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

how to check it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add log check

 # LoadExecCreateTableFailed error return twice
 err_cnt=`grep LoadExecCreateTableFailed $WORK_DIR/worker1/log/dm-worker.log | wc -l`
 if [ $err_cnt -ne 2 ]; then
     echo "error LoadExecCreateTableFailed's count is not 2"
     exit 2
 fi

@IANTHEREAL
Copy link
Collaborator

Good Job!

@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

@WangXiangUSTC
Copy link
Contributor Author

@GregoryIan @amyangfei PTAL again

loader/db.go Outdated Show resolved Hide resolved
loader/db.go Outdated Show resolved Hide resolved
loader/db.go Show resolved Hide resolved
@@ -25,6 +25,15 @@ function cleanup2() {
pkill -hup dm-tracer.test 2>/dev/null || true
}

function cleanup() {
Copy link
Contributor

@amyangfei amyangfei Jul 31, 2019

Choose a reason for hiding this comment

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

I'm afraid that cleanup cleans more data then what we need, such as it cleans running dir after a test case finished, which is not friendly for error inspection.
What about moving wait_process_exit to cleanup2, and using cleanup1 or cleanup2 when we need. Besides we can use a better name then cleanup1, cleanup2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update in 9d7936a

pkg/utils/common.go Show resolved Hide resolved
pkg/utils/common.go Show resolved Hide resolved
pkg/utils/db.go Show resolved Hide resolved
pkg/utils/db.go Outdated Show resolved Hide resolved
WangXiangUSTC and others added 4 commits July 31, 2019 11:40
Co-Authored-By: amyangfei <amyangfei@gmail.com>
Co-Authored-By: amyangfei <amyangfei@gmail.com>
@amyangfei
Copy link
Contributor

/run-all-tests

Copy link
Contributor

@amyangfei amyangfei left a comment

Choose a reason for hiding this comment

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

LGTM

@amyangfei amyangfei 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 Jul 31, 2019
@WangXiangUSTC
Copy link
Contributor Author

@GregoryIan PTAL again

@IANTHEREAL
Copy link
Collaborator

/run-all-tests

@IANTHEREAL
Copy link
Collaborator

/run-all-tests

Copy link
Collaborator

@IANTHEREAL IANTHEREAL left a comment

Choose a reason for hiding this comment

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

LGTM

@IANTHEREAL IANTHEREAL merged commit ffebdd3 into master Aug 1, 2019
@IANTHEREAL IANTHEREAL deleted the xiang/db_test branch August 1, 2019 06:11
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
priority/important Major change, requires approval from ≥2 primary reviewers status/LGT1 One reviewer already commented LGTM type/qa relate to quality assurance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants