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

s3Sink: cancel create existing table directory on s3 when initialize s3Sink #1477

Merged
merged 6 commits into from
Apr 15, 2021

Conversation

dengqee
Copy link
Contributor

@dengqee dengqee commented Mar 3, 2021

s3Sink: cancel create existing table directory on s3 when initialize s3Sink

fix bug #1463 reported

What problem does this PR solve?

fix bug #1463 reported

What is changed and how it works?

https://github.com/pingcap/ticdc/blob/355016c0ee9d689fe203db57a18449afe26271d9/cdc/sink/cdclog/s3.go#L318-L327
S3 does not create an empty directory. This code causes S3 to create objects instead of directories, like this:
image

Uploading objects to these directories will cause an error, like:

"code": "CDC:ErrProcessorUnknown",
"message": "[CDC:ErrS3SinkStorageAPI]XMinioParentIsObject: Object-prefix is already an object, please choose a different object-prefix name.\n\tstatus code: 400, request id: 166825DDCC17A799, host id: "

So I deleted the code.

Check List

Tests

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

Code changes

  • Has exported function/method change

Side effects

  • Possible performance regression

Related changes

  • Need to cherry-pick to the release branch

Release note

  • s3Sink: cancel create existing table directory on s3 when initialize s3Sink to avoid uploading objects faild

@dengqee
Copy link
Contributor Author

dengqee commented Mar 12, 2021

@3pointer @amyangfei @leoppro PTAL

@dengqee
Copy link
Contributor Author

dengqee commented Mar 17, 2021

@3pointer @amyangfei @leoppro PTAL

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.

Thanks!
I have a question about the behavior of S3, when uploading a file such as t_60/cdclog.xxxxxxxx, will the directory t_60 be created automatically?
Besides, could you please add this case to existing integration test tests/cdclog_s3

@amyangfei amyangfei added needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. status/ptal Could you please take a look? labels Mar 17, 2021
@dengqee
Copy link
Contributor Author

dengqee commented Mar 17, 2021

Thanks!
I have a question about the behavior of S3, when uploading a file such as t_60/cdclog.xxxxxxxx, will the directory t_60 be created automatically?
Besides, could you please add this case to existing integration test tests/cdclog_s3

Yes, S3 will create the directoryt_60 automatically when upload a file like t_60/cdclog.xxxxxxxx. And if you delete all files in a directory on S3, the directory will be deleted automatically.
I will try to add this case to integration test.

@amyangfei amyangfei added the component/pitr Point-in-time recovery component. label Mar 19, 2021
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

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 22, 2021
@dengqee
Copy link
Contributor Author

dengqee commented Mar 26, 2021

@3pointer @leoppro PTAL

@dengqee
Copy link
Contributor Author

dengqee commented Apr 8, 2021

@3pointer PTAL

@zier-one zier-one added the needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. label Apr 14, 2021
@3pointer
Copy link
Contributor

LGTM

@overvenus overvenus added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 15, 2021
@ti-chi-bot ti-chi-bot added status/LGT1 Indicates that a PR has LGTM 1. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Apr 15, 2021
@overvenus
Copy link
Member

/lgtm

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • overvenus

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.

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@ti-chi-bot 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 Apr 15, 2021
@overvenus
Copy link
Member

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 087c6da

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 15, 2021
@ti-chi-bot ti-chi-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 15, 2021
@amyangfei
Copy link
Contributor

/run-all-tests

@ti-chi-bot ti-chi-bot merged commit cc8eadf into pingcap:master Apr 15, 2021
ti-srebot pushed a commit to ti-srebot/ticdc that referenced this pull request Apr 15, 2021
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #1671

ti-srebot pushed a commit to ti-srebot/ticdc that referenced this pull request Apr 15, 2021
@ti-srebot
Copy link
Contributor

cherry pick to release-5.0 in PR #1672

amyangfei pushed a commit to ti-srebot/ticdc that referenced this pull request Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/pitr Point-in-time recovery component. needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. status/ptal Could you please take a look?
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants