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.

Sign up for GitHub

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

Jump to bottom

s3: add DisableMultipart option #100

Merged
merged 1 commit into from
May 12, 2024
Merged

s3: add DisableMultipart option #100

merged 1 commit into from
May 12, 2024

Conversation

fatpat
Copy link
Contributor

@fatpat fatpat commented Feb 9, 2024

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

  • s3: add DisableMultipart option to force disabling multipart all together.

Verification

Checked that the uploaded object was not upload with multipart

@fatpat fatpat mentioned this pull request Feb 10, 2024
Open
4 tasks
@fatpat
Copy link
Contributor Author

fatpat commented Feb 22, 2024

anyone to look at this please ?

@fatpat
Copy link
Contributor Author

fatpat commented Feb 26, 2024

I'd like a review if that's possible please :)
thx

@yeya24 @saswatamcode @brancz

@yeya24
Copy link
Contributor

yeya24 commented Feb 27, 2024

Hi @fatpat, thanks for the contribution. May I know the usecase for disabling it? And maybe it is worth checking the conflicts since I just merged another pr.

@fatpat
Copy link
Contributor Author

fatpat commented Feb 28, 2024 •
edited
Loading

Hi @fatpat, thanks for the contribution. May I know the usecase for disabling it? And maybe it is worth checking the conflicts since I just merged another pr.

Hello @yeya24,

I just rebase.

The motivation comes from a need in mimir that use objstore as a dependency. For the motivation I'll copy the justifcation I wrote on the mimir PR 7350:

we discovered that we could achieve better GET performances from our internal objecstorage with multipart disabled.

our use case is a high load with a high frequency (1s interval). We were trying to play with the part size while we discovered it was not possible do disable it all together (thanos-io/objstore resets part size if the object size is higher than the part size).

Moreover all S3 clients have an option to disable multi-part uploads. Not having the option in mimir looks like something was missing.

yeya24
yeya24 approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks

@fatpat
Copy link
Contributor Author

fatpat commented Mar 1, 2024 •
edited
Loading

Thanks

Thanks to you for approving.

Who can click on the merge button? 👌

@yeya24
Copy link
Contributor

yeya24 commented Mar 1, 2024

I am waiting to see if I can get another pair of eyes on this...
Maybe @fpetkovski @kakkoyun @saswatamcode?

fpetkovski
fpetkovski approved these changes Mar 4, 2024
View reviewed changes
README.md
@@ -181,6 +181,7 @@ config:
list_objects_version: ""
bucket_lookup_type: auto
send_content_md5: true
disable_multipart: false
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit unfortunate that we have to use a double negation, but I don't see another way to preserve backwards compatibility.

@fatpat
Copy link
Contributor Author

fatpat commented May 12, 2024

any news on this ? @yeya24 ?

@yeya24 yeya24 enabled auto-merge May 12, 2024 04:50
@yeya24
Copy link
Contributor

yeya24 commented May 12, 2024

Sorry I forgot to merge it. Thanks for the reminder.

@yeya24 yeya24 disabled auto-merge May 12, 2024 05:30
@yeya24 yeya24 enabled auto-merge May 12, 2024 05:30
@yeya24 yeya24 disabled auto-merge May 12, 2024 05:31
@fatpat
Copy link
Contributor Author

fatpat commented May 12, 2024

Sorry I forgot to merge it. Thanks for the reminder.

do you want me to rebase the PR ?

@yeya24
Copy link
Contributor

yeya24 commented May 12, 2024

I am not sure why this pr didn't get auto merged. Maybe rebase will help

@fatpat
s3: add DisableMultipart option
24f3dea 
Signed-off-by: Jérôme LOYET <822436+fatpat@users.noreply.github.com>
@fatpat
Copy link
Contributor Author

fatpat commented May 12, 2024

I am not sure why this pr didn't get auto merged. Maybe rebase will help

done

@yeya24 yeya24 merged commit 71ef2d0 into thanos-io:main May 12, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fpetkovski fpetkovski fpetkovski approved these changes

@yeya24 yeya24 yeya24 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fatpat @yeya24 @fpetkovski