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

Switched paths in S3 metadata to relative #11892

Merged
merged 1 commit into from
Jul 6, 2020
Merged

Switched paths in S3 metadata to relative #11892

merged 1 commit into from
Jul 6, 2020

Conversation

excitoon
Copy link
Contributor

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • New Feature

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

Switched paths in S3 metadata to relative which allows to handle S3 blobs more easily.

@blinkov blinkov added doc-alert pr-feature Pull request with new product feature labels Jun 23, 2020
@Akazz Akazz self-assigned this Jun 23, 2020
@alesapin alesapin added the st-waiting-for-fix We are waiting for fixes in this issue or pull request label Jun 26, 2020
Copy link
Contributor

@Akazz Akazz left a comment

Choose a reason for hiding this comment

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

This changeset looks okay in general.

Although you might want to improve Metadata class in the future: to entirely encapsulate s3_root and return objectIds (which are simply random numbers) from addObject(). This way the class would be semantically more consistent and so you do not need to worry about generating names outside.

P.S. It looks like WriteIndirectBufferFromS3::finalize() is not exception-safe and can leak remote blob space (if metadata.save() fails). Perhaps it is worth putting a comment/TODO in about it.

@Akazz
Copy link
Contributor

Akazz commented Jul 6, 2020

Performance tests failures seem unrelated completely.
fyi, @akuzm

@Akazz Akazz removed the st-waiting-for-fix We are waiting for fixes in this issue or pull request label Jul 6, 2020
@Akazz Akazz merged commit 1c0c8a7 into ClickHouse:master Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature Pull request with new product feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants