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

[to #215] Add integration test for tikv-cdc #216

Merged
merged 15 commits into from
Sep 1, 2022
Merged

[to #215] Add integration test for tikv-cdc #216

merged 15 commits into from
Sep 1, 2022

Conversation

zeminzhou
Copy link
Contributor

What problem does this PR solve?

Add integration test for tikv-cdc

Issue Number: close #215

Problem Description: TBD
TiKV-CDC needs integraion test

What is changed and how does it work?

Add integration test for tikv-cdc

Check List for Tests

This PR has been tested by at least one of the following methods:

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

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
@codecov
Copy link

codecov bot commented Aug 29, 2022

Codecov Report

Merging #216 (43d489b) into main (8cf6529) will increase coverage by 0.5476%.
The diff coverage is 100.0000%.

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff                @@
##               main       #216        +/-   ##
================================================
+ Coverage   61.3078%   61.8555%   +0.5476%     
================================================
  Files           170        170                
  Lines         13809      13829        +20     
================================================
+ Hits           8466       8554        +88     
+ Misses         4716       4630        -86     
- Partials        627        645        +18     
Flag Coverage Δ
cdc 61.8555% <100.0000%> (+0.5476%) ⬆️

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

Impacted Files Coverage Δ
cdc/cdc/sink/metrics.go 0.0000% <ø> (ø)
cdc/cdc/sink/statistics.go 52.8571% <100.0000%> (+2.1108%) ⬆️
cdc/cdc/sink/tikv.go 81.7796% <100.0000%> (+1.0621%) ⬆️
cdc/pkg/util/key.go 71.4285% <100.0000%> (+1.7316%) ⬆️
cdc/cdc/sorter/unified/unified_sorter.go 92.3611% <0.0000%> (ø)
cdc/cdc/sorter/unified/merger.go 68.2847% <0.0000%> (+0.6472%) ⬆️
cdc/cdc/kv/client.go 86.3112% <0.0000%> (+2.1613%) ⬆️
cdc/pkg/retry/retry_with_opt.go 95.4545% <0.0000%> (+4.5454%) ⬆️
cdc/pkg/pdtime/acquirer.go 65.1162% <0.0000%> (+6.9767%) ⬆️
cdc/cdc/sorter/unified/file_backend.go 53.9473% <0.0000%> (+20.1754%) ⬆️

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
@zeminzhou
Copy link
Contributor Author

@pingyu @haojinming PTAL~

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
cdc/tests/integration_tests/run.sh Outdated Show resolved Hide resolved
cdc/tests/utils/rawkv_data/main.go Outdated Show resolved Hide resolved
cdc/tests/utils/rawkv_data/gen_data.go Outdated Show resolved Hide resolved
cdc/tests/utils/rawkv_data/gen_data.go Outdated Show resolved Hide resolved
cdc/tests/utils/rawkv_data/gen_data.go Outdated Show resolved Hide resolved
cdc/tests/utils/rawkv_data/gen_data.go Outdated Show resolved Hide resolved
cdc/tests/utils/rawkv_data/gen_data.go Outdated Show resolved Hide resolved
cdc/tests/utils/rawkv_data/gen_data.go Outdated Show resolved Hide resolved
cdc/tests/utils/rawkv_data/checksum.go Outdated Show resolved Hide resolved
Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
Copy link
Collaborator

@pingyu pingyu 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.

PWD=$(pwd)
if [ ! -f $binary ]; then
cd $CUR/../../utils/rawkv_data/
GO111MODULE=on go build
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest to build by Makefile, to make build flags consistent.

PWD=$(pwd)

cd $CUR/../../utils/rawkv_data
if [ ! -f ./rawkv_data ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest to build by Makefile, to make build flags consistent.

Then we don't need this script ? Add utils/rawkv_data to $PATH and use it directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use Makefile to build rawkv_data, but I don't remove this script that used to check if rawkv_data exist.

@@ -219,15 +221,6 @@ tidb-server \
--status=${UP_TIDB_STATUS} \
--log-file "$OUT_DIR/tidb.log" &

randomGenSocketsConf
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest to use tiup playground to start the cluster, which would be easier, as we don't need to maintain all the commands & ports here. We just concern ports of PD.

But in current version of tiup playground, the PD port of downstream cluster is random (as the default 2379 is occupied by upstream cluster), which caused it difficult to make automatic tests.

The good news is that the feature of specifying PD port will be released in a near version (v1.11.0, see pingcap/tiup#1931).

So I think we can consider change to tiup playground later after the new version is released.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If use tiup playground, we can't add/remove cdc_server casually for next test case.

Copy link
Contributor

@haojinming haojinming left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
Copy link
Collaborator

@pingyu pingyu left a comment

Choose a reason for hiding this comment

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

LGTM~

@pingyu pingyu merged commit 824a451 into tikv:main Sep 1, 2022
@zeminzhou zeminzhou deleted the it branch October 17, 2022 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TiKV-CDC needs integration test
3 participants