Skip to content

Fixes for objects removal in S3ObjectStorage#37882

Merged
rschu1ze merged 5 commits intoClickHouse:masterfrom
excitoon-favorites:nodeleteobjects
Jul 12, 2022
Merged

Fixes for objects removal in S3ObjectStorage#37882
rschu1ze merged 5 commits intoClickHouse:masterfrom
excitoon-favorites:nodeleteobjects

Conversation

@excitoon
Copy link
Copy Markdown
Contributor

@excitoon excitoon commented Jun 6, 2022 •
edited
Loading

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

S3 single objects are now removed with RemoveObjectRequest (sic). Fixed a bug with S3ObjectStorage on GCP which did not allow to use removeFileIfExists effectively breaking approximately half of remove functionality. Automatic detection for DeleteObjects S3 API, that is not supported by GCS. This will allow to use GCS without explicit support_batch_delete=0 in configuration.

@robot-ch-test-poll robot-ch-test-poll added the pr-improvement Pull request with some product improvements label Jun 6, 2022
@excitoon
Copy link
Copy Markdown
Contributor Author

excitoon commented Jun 9, 2022

@alz said it is worth making this option to determine automatically.

@alex-zaitsev
Copy link
Copy Markdown
Contributor

alex-zaitsev commented Jun 16, 2022

Related to #37659

@excitoon excitoon force-pushed the nodeleteobjects branch 2 times, most recently from 8b010ea to 55fe159 Compare June 23, 2022 08:42
@excitoon excitoon changed the title has_delete_objects setting for S3 disk Fixes for objects removal in S3ObjectStorage Jun 23, 2022
@excitoon excitoon changed the title Fixes for objects removal in S3ObjectStorage Fixes for objects removal in S3ObjectStorage Jun 23, 2022
@excitoon excitoon marked this pull request as ready for review June 23, 2022 08:42
@rschu1ze rschu1ze self-assigned this Jun 28, 2022
@rschu1ze rschu1ze added the can be tested Allows running workflows for external contributors label Jun 28, 2022
rschu1ze
rschu1ze reviewed Jun 28, 2022
View reviewed changes
@rschu1ze
Copy link
Copy Markdown
Member

rschu1ze commented Jun 28, 2022

clang-tidy failure (BuilderBinTidy) is related, besides that this PR lgtm.

rschu1ze
rschu1ze reviewed Jun 29, 2022
View reviewed changes
@excitoon

This comment was marked as resolved.

Sign in to view
@rschu1ze

This comment was marked as resolved.

Sign in to view
@excitoon

This comment was marked as resolved.

Sign in to view
@rschu1ze

This comment was marked as resolved.

Sign in to view
@azat azat mentioned this pull request Jun 30, 2022
Merged
azat
azat reviewed Jun 30, 2022
View reviewed changes
azat
azat reviewed Jun 30, 2022
View reviewed changes
@azat azat mentioned this pull request Jun 30, 2022
Closed
@azat
Copy link
Copy Markdown
Member

azat commented Jun 30, 2022

@excitoon AFAIR lists are not parsed correctly in changelog entries, and I doubt that "better removal" important for user, so maybe something like this will be better and enough "Automatic detection for DeleteObjects S3 API, that is not supported by GCS. This will allow to use GCS without explicit support_batch_delete=0 in configuration"

@excitoon excitoon requested a review from azat June 30, 2022 23:07
rschu1ze
rschu1ze reviewed Jul 1, 2022
View reviewed changes
@rschu1ze

This comment was marked as outdated.

Sign in to view
@excitoon
Copy link
Copy Markdown
Contributor Author

excitoon commented Jul 11, 2022 •
edited
Loading

@rschu1ze I'm done with the tests

@rschu1ze
Copy link
Copy Markdown
Member

rschu1ze commented Jul 11, 2022 •
edited
Loading

Thanks. Tests look good, it just complains about a style check for the Python test ... would you like to fix that cosmetic issue (I have seen PRs being rolled back due to style issues 😕) ?

@excitoon
Copy link
Copy Markdown
Contributor Author

excitoon commented Jul 11, 2022

@rschu1ze Sure I can. BTW, what is the point of this style check if it could be automatically applied on my changes?

@excitoon
Copy link
Copy Markdown
Contributor Author

excitoon commented Jul 11, 2022

@rschu1ze Fixed!

@rschu1ze
Copy link
Copy Markdown
Member

rschu1ze commented Jul 11, 2022

Thanks! Will merge as soon as the final round of tests is done.

(My guess is that "automatically" fixing commits opens a can of worms ... starting from authorization problems (the style checker needs to have GitHub credentials) up to legal problems, e.g. does the originals authors CLA also cover an automatically "fixed commit". Really don't know what kind of edge cases are out there. But I agree that there should at least be a way of semi-automatically fixing style, e.g. via a Git hook integration ...)

@rschu1ze
Copy link
Copy Markdown
Member

rschu1ze commented Jul 12, 2022

Remaining test failures are unrelated and addressed in a different PR --> merged

@rschu1ze rschu1ze merged commit 8b6e31c into ClickHouse:master Jul 12, 2022
@UnamedRus UnamedRus mentioned this pull request Jul 13, 2022
Closed
excitoon pushed a commit to Altinity/ClickHouse that referenced this pull request Jul 14, 2022
@rschu1ze @excitoon
Merge pull request ClickHouse#37882 from excitoon-favorites/nodeleteo…
d45018f 
…bjects

Fixes for objects removal in `S3ObjectStorage`
@excitoon excitoon mentioned this pull request Jul 14, 2022
Merged
arthurpassos added a commit to Altinity/ClickHouse that referenced this pull request Jul 15, 2022
@arthurpassos
Merge pull request #164 from Altinity/backport/22.3_36944_34632
97b66d6 
Window functions and GCP fixes ClickHouse#36944, ClickHouse#34632, ClickHouse#37659 and ClickHouse#37882
Enmk pushed a commit to Altinity/ClickHouse that referenced this pull request Jul 28, 2022
@rschu1ze @Enmk
Merge pull request ClickHouse#37882 from excitoon-favorites/nodeleteo…
600808b 
…bjects

Fixes for objects removal in `S3ObjectStorage`
Enmk pushed a commit to Altinity/ClickHouse that referenced this pull request Jul 28, 2022
@arthurpassos @Enmk
Merge pull request #164 from Altinity/backport/22.3_36944_34632
38f7435 
Window functions and GCP fixes ClickHouse#36944, ClickHouse#34632, ClickHouse#37659 and ClickHouse#37882
@Enmk Enmk mentioned this pull request Jul 28, 2022
Closed
Enmk pushed a commit to Altinity/ClickHouse that referenced this pull request Aug 4, 2022
@rschu1ze @Enmk
Merge pull request ClickHouse#37882 from excitoon-favorites/nodeleteo…
e336a8b 
…bjects

Fixes for objects removal in `S3ObjectStorage`
Enmk pushed a commit to Altinity/ClickHouse that referenced this pull request Aug 4, 2022
@arthurpassos @Enmk
Merge pull request #164 from Altinity/backport/22.3_36944_34632
797f3d3 
Window functions and GCP fixes ClickHouse#36944, ClickHouse#34632, ClickHouse#37659 and ClickHouse#37882
This was referenced Aug 4, 2022
Closed
Merged
@Enmk Enmk mentioned this pull request Sep 12, 2022
Merged
4 tasks
Enmk pushed a commit to Altinity/ClickHouse that referenced this pull request Sep 13, 2022
@rschu1ze @Enmk
Merge pull request ClickHouse#37882 from excitoon-favorites/nodeleteo…
9fc95f3 
…bjects

Fixes for objects removal in `S3ObjectStorage`
Enmk pushed a commit to Altinity/ClickHouse that referenced this pull request Sep 17, 2022
@novikd @Enmk
PR #164 Window functions and GCP fixes ClickHouse#36944, ClickHouse#3…
d80e330 
…4632, ClickHouse#37659 and ClickHouse#37882

Merge pull request ClickHouse#36944 from excitoon-favorites/better_exp_smooth
Merge pull request ClickHouse#34632 from excitoon-favorites/optimizedprocessing
Merge pull request ClickHouse#37659 from frew/master
Support `batch_delete` capability for GCS
Merge pull request ClickHouse#37882 from excitoon-favorites/nodeleteobjects
Fixes for objects removal in `S3ObjectStorage`
@Enmk Enmk mentioned this pull request Dec 22, 2022
Merged
@DanRoscigno DanRoscigno mentioned this pull request Jan 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rschu1ze rschu1ze rschu1ze approved these changes

@azat azat Awaiting requested review from azat

Assignees

@rschu1ze rschu1ze

Labels

can be tested Allows running workflows for external contributors pr-improvement Pull request with some product improvements

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@excitoon @alex-zaitsev @rschu1ze @azat @robot-ch-test-poll