fix(deps): update github.com/thanos-io/objstore digest to 37752ee (main) #6622
Conversation
![renovate[bot]](https://avatars.githubusercontent.com/in/2740?s=60&v=4){kind=small}
| @@ -510,6 +514,7 @@ func (b *Bucket) Upload(ctx context.Context, name string, r io.Reader) error { | |||
| ServerSideEncryption: sse, | |||
| UserMetadata: userMetadata, | |||
| StorageClass: b.storageClass, | |||
| SendContentMd5: b.sendContentMd5, | |||
There was a problem hiding this comment.
We should double-check what's the implication of setting SendContentMd5 to true by default. Does it mean we will use CPU to compute MD5 of uploaded objects?
There was a problem hiding this comment.
Does it mean we will use CPU to compute MD5 of uploaded objects?
I believe so (I have not stepped through or ran tests to confirm). In minio's code there are comments like this: // CRC32C is ~50% faster on AMD64 @ 30GB/s.
The motivation for the objstore PR seems to have been compatibility by default with Backblaze B2. On our side the currently open #6592 seems to be related to the same issue (but with a more indirect solution).
I think exposing the option makes sense for us now, but choosing the default in this situation is tricky since both options have downsides. true adds a hidden cost to everyone (it's not required), false adds a visible and correctable cost to some. Looking even further ahead, there's trailing headers (thanos-io/objstore#57) to keep in mind.
Seems like a tough call. I'm slightly more in favor of false.
There was a problem hiding this comment.
My understanding is that it was false until now, so I'd just keep current behaviour, and perhaps expose an option to change it by people who need it. WDYT?
There was a problem hiding this comment.
Minio warns that SendContentMd5 will cause higher memory usage "because of in-memory md5sum calculation". Also the code comment (linked to by Andy) states 50% worse perf with MD5 over CRC32. I would default to false, as that should be the behaviour before this PR, but we could expose the option.
There was a problem hiding this comment.
My understanding is that it was false until now, so I'd just keep current behaviour
I would default to false, as that should be the behaviour before this PR
Seems like there's agreement on false then 👍
As an aside my understanding was that the past behavior had already changed in some way like:
- Minio not using MD5 or checksum when we didn't set
trueto send an MD5 - Minio starts using checksums when not sending an MD5 <-- breakage for some users happens here
- Now
Oddly the minio issue related to this was from quite a while ago. The Thanos bump pointed to in the objstore PR doesn't include a minio upgrade at all according to the go.sum. I feel like there's some confusion somewhere regarding what change actually caused this to surface more recently, but even if that archaeology was performed I don't think it changes anything for us.
There was a problem hiding this comment.
Minio starts using checksums when not sending an MD5 <-- breakage for some users happens here
Yeah that was also my understanding from digging into the backstory (CRC checksum + an Amazon checksum header). It's confusing, and not well communicated 😞
There was a problem hiding this comment.
Added -<prefix>.s3.send-content-md5 defaulting to false.
There was a problem hiding this comment.
Do we need to support this MD5 thing at all in Mimir?
renovate
bot
changed the title
renovate
bot
changed the title
renovate
bot
changed the title
renovate
bot
changed the title
Edited/Blocked NotificationRenovate will not automatically rebase this PR, because it does not recognize the last commit author and assumes somebody else may have edited the PR. You can manually request rebase by checking the rebase/retry box above. ⚠ Warning: custom changes will be lost. |
andyasp
deleted the
deps-update/main-github.com-thanos-io-objstore-digest
branch
This PR contains the following updates:
ff7faac->37752eeConfiguration
📅 Schedule: Branch creation - "before 9am on Monday" (UTC), Automerge - At any time (no schedule defined).
🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.
♻ Rebasing: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.
🔕 Ignore: Close this PR and you won't be reminded about this update again.
This PR has been generated by Mend Renovate. View repository job log here.