Fix s3 upload performance regression #98
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The symptom
The PR #66 introduced a regression in the S3 upload performance. After that commit, S3 uploaded are significantly slower. Here you can see how the upload latency changed for one of our Grafana Mimir clusters running in AWS:
Change deployed to prod on 10/06, where the latency increase. Since the upload performance in Mimir is not critical (e.g. doesn't affect neither read and write path latency), it was undetected for quite some time. Until today.
## The cause
Why the regression? Minio has some optimizations when the reader implements
io.ReaderAt
because it can leverage on.ReadAt()
. We're used to callUpload()
withos.File
as a reader (I think Thanos does the same). After #66 wrapped the reader with the timing reader, which doesn't implementio.ReaderAt
and so Minio couldn't optimize the upload anymore.The fix
In this PR I propose a fix to the issue, introducing
timingReaderSeekerReaderAt
which embedstimingReaderSeeker
, which in turn embedstimingReader
.To simplify the code, I've removed the usage of
nopSeekerCloserWithSize()
andNopCloserWithSize()
in theUpload
and pushed down totimingReader
whether we want it toClose()
the wrapped reader or not. The renaming fromtimingReadCloser
totimingReader
follows this change, given it takes in inputio.Reader
.Manual test
I've manually that this PR fixes the upload speed regression, running a test in AWS which uploads 1GB object to S3 multiple times.
Before this PR
S3 bucket not wrapped with metrics:
S3 bucket wrapped with metrics (see higher latency):
After this PR
S3 bucket not wrapped with metrics:
S3 bucket wrapped with metrics (see no higher latency):