dumpling: compress supports snappy/zstd by lyzx2001 · Pull Request #38910 · pingcap/tidb

lyzx2001 · 2022-11-04T10:48:33Z

What problem does this PR solve?

Issue Number: ref #38514

Problem Summary:

What is changed and how it works?

Dumpling compress supports snappy/zstd

Check List

Tests

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

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

ti-chi-bot · 2022-11-04T10:48:35Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

lance6716
lichunzhu

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

lichunzhu · 2022-11-07T09:03:22Z

Is this PR ready for review now?

lyzx2001 · 2022-11-07T09:16:51Z

Is this PR ready for review now?

there are still 2 tests failing and I am trying to fix them

lyzx2001 · 2022-11-09T07:43:03Z

/cc @lichunzhu @lance6716

lichunzhu

Rest LGTM

lichunzhu · 2022-11-09T08:31:18Z

dumpling/export/config.go

 		return storage.Gzip, nil
+	case "snappy":
+		return storage.Snappy, nil
+	case "zst":


Do you mean this? It's different with the description in comment

Suggested change

case "zst":

case "zstd":

or maybe

Suggested change

case "zst":

case "zstd","zst":

lichunzhu · 2022-11-09T08:33:40Z

dumpling/tests/s3/run.sh

+# test snappy
+run_dumpling --s3.endpoint="http://$S3_ENDPOINT/" --compress "snappy"
+
+bin/mc cp minio/mybucket/dump-compress/s3-schema-create.sql.snappy "${HOST_DIR}/compress/s3-schema-create.sql.snappy"
+bin/mc cp minio/mybucket/dump-compress/s3.t-schema.sql.snappy "${HOST_DIR}/compress/s3.t-schema.sql.snappy"
+bin/mc cp minio/mybucket/dump-compress/s3.t.000000000.sql.snappy "${HOST_DIR}/compress/s3.t.000000000.sql.snappy"
+
+snappy -d "${HOST_DIR}/compress/s3-schema-create.sql.snappy"
+diff "${HOST_DIR}/local/s3-schema-create.sql" "${HOST_DIR}/compress/s3-schema-create.sql"
+
+snappy -d "${HOST_DIR}/compress/s3.t-schema.sql.snappy"
+diff "${HOST_DIR}/local/s3.t-schema.sql" "${HOST_DIR}/compress/s3.t-schema.sql"
+
+snappy -d "${HOST_DIR}/compress/s3.t.000000000.sql.snappy"
+diff "${HOST_DIR}/local/s3.t.000000000.sql" "${HOST_DIR}/compress/s3.t.000000000.sql"
+
+rm "${HOST_DIR}/compress/s3-schema-create.sql"
+rm "${HOST_DIR}/compress/s3.t-schema.sql"
+rm "${HOST_DIR}/compress/s3.t.000000000.sql"
+
+# test zstd
+run_dumpling --s3.endpoint="http://$S3_ENDPOINT/" --compress "zst"
+
+bin/mc cp minio/mybucket/dump-compress/s3-schema-create.sql.zst "${HOST_DIR}/compress/s3-schema-create.sql.zst"
+bin/mc cp minio/mybucket/dump-compress/s3.t-schema.sql.zst "${HOST_DIR}/compress/s3.t-schema.sql.zst"
+bin/mc cp minio/mybucket/dump-compress/s3.t.000000000.sql.zst "${HOST_DIR}/compress/s3.t.000000000.sql.zst"
+
+zstd "${HOST_DIR}/compress/s3-schema-create.sql.zst" -d
+diff "${HOST_DIR}/local/s3-schema-create.sql" "${HOST_DIR}/compress/s3-schema-create.sql"
+
+zstd "${HOST_DIR}/compress/s3.t-schema.sql.zst" -d
+diff "${HOST_DIR}/local/s3.t-schema.sql" "${HOST_DIR}/compress/s3.t-schema.sql"
+
+zstd "${HOST_DIR}/compress/s3.t.000000000.sql.zst" -d
+diff "${HOST_DIR}/local/s3.t.000000000.sql" "${HOST_DIR}/compress/s3.t.000000000.sql"


Can we merge these three parts in a for loop?

lyzx2001 · 2022-11-10T07:11:23Z

/run-all-tests

lichunzhu

Great job! LGTM

lance6716 · 2022-11-10T10:30:24Z

/merge

ti-chi-bot · 2022-11-10T10:30:27Z

@lance6716: /merge in this pull request requires 2 approval(s).

Details

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

lance6716 · 2022-11-10T10:33:10Z

will review later

lance6716

rest lgtm

lance6716 · 2022-11-11T08:32:47Z

dumpling/install.sh

 wget https://dl.minio.io/client/mc/release/linux-amd64/mc -O bin/mc
 chmod a+x bin/mc
+
+go get github.com/ma6174/snappy


Suggested change

go get github.com/ma6174/snappy

only need go install?

error encountered in CI integration test

https://github.com/pingcap/tidb/actions/runs/3444134903/jobs/5746413416#step:6:7138

i think maybe should add back go get github.com/ma6174/snappy?

lance6716 · 2022-11-11T08:35:32Z

dumpling/tests/s3/run.sh

+
+  case $compressType in
+  "gzip")
+    gzip "${HOST_DIR}/compress/s3-schema-create.sql.gz" -d


seems we can assign gzip, snappy, zst to $binary, and also assign $suffix, then use

${binary} "${HOST_DIR}/compress/s3-schema-create.sql.${suffix}" -d diff "${HOST_DIR}/local/s3-schema-create.sql" "${HOST_DIR}/compress/s3-schema-create.sql" ${binary} "${HOST_DIR}/compress/s3.t-schema.sql.${suffix}" -d diff "${HOST_DIR}/local/s3.t-schema.sql" "${HOST_DIR}/compress/s3.t-schema.sql" ${binary} "${HOST_DIR}/compress/s3.t.000000000.sql.${suffix}" -d diff "${HOST_DIR}/local/s3.t.000000000.sql" "${HOST_DIR}/compress/s3.t.000000000.sql"

lyzx2001 · 2022-11-11T12:30:38Z

/run-all-tests

lyzx2001 · 2022-11-11T12:38:45Z

/run-all-tests

lyzx2001 · 2022-11-11T13:34:01Z

/run-all-tests

lyzx2001 · 2022-11-12T04:03:30Z

/run-all-tests

lichunzhu · 2022-11-14T03:11:03Z

/merge

ti-chi-bot · 2022-11-14T03:11:07Z

This pull request has been accepted and is ready to merge.

Details

Commit hash: 5ba6b50

sre-bot · 2022-11-14T05:16:39Z

TiDB MergeCI notify

🔴 Bad News! New failing [3] after this pr merged.
These new failed integration tests seem to be caused by the current PR, please try to fix these new failed integration tests, thanks!

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-ddl-test	🟥 failed 1, success 5, total 6	43 min	New failing
idc-jenkins-ci/integration-cdc-test	🟥 failed 1, success 38, total 39	22 min	New failing
idc-jenkins-ci-tidb/integration-common-test	🟥 failed 1, success 16, total 17	10 min	New failing
idc-jenkins-ci-tidb/mybatis-test	🔴 failed 1, success 0, total 1	11 min	Existing failure
idc-jenkins-ci-tidb/common-test	🟢 all 11 tests passed	8 min 51 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	6 min 45 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	4 min 30 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	4 min 22 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	2 min 40 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

dumpling: compress supports snappy/zstd

88a8841

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 4, 2022

solve conflicts

d3443f1

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2022

lyzx2001 added 14 commits November 4, 2022 18:59

dumpling: compress supports snappy/zstd

4fbb17c

dumpling: compress supports snappy/zstd

8d15c12

dumpling: compress supports snappy/zstd

cff6ee6

dumpling: compress supports snappy/zstd

f5e11ac

dumpling: compress supports snappy/zstd

cfa6563

dumpling: compress supports snappy/zstd

5256527

modify go.mod and go.sum

628f704

modify install.sh

fe1e21d

modify install.sh

2626711

modify install.sh

12c54b8

modify install.sh

7a81d30

modify install.sh

8c158cc

modify install.sh

fc5281c

modify install.sh

b067c88

modify install.sh

48fa4db

ti-chi-bot requested review from lance6716 and lichunzhu November 9, 2022 07:43

lichunzhu reviewed Nov 9, 2022

View reviewed changes

dumpling: compress supports snappy/zstd

808277c

Merge branch 'master' into dumpling-compress

d2b7560

lichunzhu approved these changes Nov 10, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 10, 2022

lichunzhu added the component/dumpling This is related to Dumpling of TiDB. label Nov 10, 2022

lyzx2001 added 4 commits November 10, 2022 21:40

improve backward compatibility of 'SHOW SLAVE HOSTS' command

a6accde

dumpling: compress supports snappy/zstd

40e0584

dumpling: compress supports snappy/zstd

5c7072e

dumpling: compress supports snappy/zstd

f303c95

lance6716 reviewed Nov 11, 2022

View reviewed changes

lyzx2001 added 2 commits November 11, 2022 16:54

Merge branch 'master' into dumpling-compress

bd2de30

dumpling: compress supports snappy/zstd

556bb36

lance6716 approved these changes Nov 11, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 11, 2022

dumpling: compress supports snappy/zstd

bdc2814

Merge branch 'master' into dumpling-compress

5ba6b50

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 14, 2022

Merge branch 'master' into dumpling-compress

f43b8af

ti-chi-bot merged commit 78d1905 into pingcap:master Nov 14, 2022

Conversation

lyzx2001 commented Nov 4, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lichunzhu commented Nov 7, 2022

Uh oh!

lyzx2001 commented Nov 7, 2022

Uh oh!

lyzx2001 commented Nov 9, 2022

Uh oh!

lichunzhu left a comment

Choose a reason for hiding this comment

Uh oh!

lichunzhu Nov 9, 2022

Choose a reason for hiding this comment

Uh oh!

lichunzhu Nov 9, 2022

Choose a reason for hiding this comment

Uh oh!

lyzx2001 commented Nov 10, 2022

Uh oh!

lichunzhu left a comment

Choose a reason for hiding this comment

Uh oh!

lance6716 commented Nov 10, 2022

Uh oh!

ti-chi-bot commented Nov 10, 2022

Uh oh!

lance6716 commented Nov 10, 2022

Uh oh!

lance6716 left a comment

Choose a reason for hiding this comment

Uh oh!

lance6716 Nov 11, 2022

Choose a reason for hiding this comment

Uh oh!

lyzx2001 Nov 11, 2022

Choose a reason for hiding this comment

Uh oh!

lance6716 Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyzx2001 Nov 11, 2022

Choose a reason for hiding this comment

Uh oh!

lyzx2001 commented Nov 11, 2022

Uh oh!

lyzx2001 commented Nov 11, 2022

Uh oh!

lyzx2001 commented Nov 11, 2022

Uh oh!

lyzx2001 commented Nov 12, 2022

Uh oh!

lichunzhu commented Nov 14, 2022

Uh oh!

ti-chi-bot commented Nov 14, 2022

Uh oh!

sre-bot commented Nov 14, 2022

TiDB MergeCI notify

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ti-chi-bot commented Nov 4, 2022 •

edited

Loading

lance6716 Nov 11, 2022 •

edited

Loading