Add support for storage class configuration #1714
Conversation
|
|
amitsetty
requested review from
joe-elliott,
annanay25,
mdisibio,
mapno,
kvrhdn and
zalegrala
as code owners
tempodb/backend/s3/s3.go
Outdated
| options := minio.PutObjectOptions{ | ||
| PartSize: rw.cfg.PartSize, | ||
| } | ||
| options := getTempoObjectOptions(rw) |
There was a problem hiding this comment.
This does add UserTags here which feels like a bug fix. Can you think of any reason we wouldn't want that?
There was a problem hiding this comment.
Thought about it a little.
This will just set the user tags to the new user tags during append.
Sounds to me like something that leads to a negligible performance impact and will only lead to a behavior change if someone changes the user tags (which in this case will lead to the new user tags when appending)
Seems to me that if anything this is a good thing unless there is a performance change. Do you think that this could lead to a performance change? If so I'll be more than happy to change it
There was a problem hiding this comment.
Agree it seems like a great bug fix. It doesn't look like the headers are supported on UploadPart but they are supported on CreateMultipartUpload. Since it's working then either the extra headers don't matter or the minio client is handling it. Have you ran this against real S3 and seen any issues?
There was a problem hiding this comment.
Haven't ran it against a real S3. Feel free to check and I'll make any changes necessary :)
amitsetty
requested review from
mdisibio and
joe-elliott
and removed request for
mdisibio
amitsetty
requested review from
mdisibio
and removed request for
joe-elliott
What this PR does: Adds support for storage class configuration
Which issue(s) this PR fixes:
Fixes #1684
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]