Limit the maximum size of block uploads #4680
Conversation
pkg/compactor/block_upload.go
Outdated
|
|
||
| if blockSizeBytes > maxBlockSizeBytes || blockSizeBytes < 0 { | ||
| err := fmt.Errorf(maxBlockUploadSizeBytesFormat, maxBlockSizeBytes) | ||
| level.Error(logger).Log("err", err, "size", blockSizeBytes) |
There was a problem hiding this comment.
wouldn't this error be also logged here?
mimir/pkg/compactor/block_upload.go
Line 313 in bf1be65
There was a problem hiding this comment.
WDYT about making this option advanced?
All for it, thanks I hadn't thought about the category when originally writing this.
wouldn't this error be also logged here?
I'll look into this. The only reason I added this log line was to give visibility into the observed block size, but I didn't want to communicate that back to the user.
There was a problem hiding this comment.
I'll look into this. The only reason I added this log line was to give visibility into the observed block size, but I didn't want to communicate that back to the user.
i wouldn't mind having a more verbose error for the user. I was more concerned about noisy logging. But not feeling strongly about this, so however you decide
There was a problem hiding this comment.
Agree that it seems unnecessary to hide computed block size from user.
There was a problem hiding this comment.
I generally defer to providing less information in errors to reduce adding surface area to an API. I thought logging on the server side was more useful because the rejections could be analyzed all together, but the information provided to the user is less relevant and possibly even confusing (the metadata size isn't counted, an overflow would return a negative number and may not be representative of the entire sum).
There was a problem hiding this comment.
I generally defer to providing less information in errors to reduce adding surface area to an API. I thought logging on the server side was more useful because the rejections could be analyzed all together, but the information provided to the user is less relevant and possibly even confusing (the metadata size isn't counted, an overflow would return a negative number and may not be representative of the entire sum).
I don't mind if we report overflown value to the user (realistically that will never happen for "normal" blocks), and we can avoid confusion by better wording saying that it doesn't include block metadata.
The point is that we already log errors returned to user, so why bother logging it once more with extra details, if this detail is something user can figure out themselves and is not sensitive in any way.
(But this is a nit, we can keep it as is)
There was a problem hiding this comment.
I was thinking about this some more. Rather than returning the size of the rejection maybe a histogram of block sizes proposed in start upload would be generally more useful. If it included rejected sizes as well it could provide insight on what the limit could be moved to without the burden of interpreting log lines/errors. Overflow might have to be excluded to avoid negatives though, but that seems fine.
There was a problem hiding this comment.
Rather than returning the size of the rejection maybe a histogram of block sizes proposed in start upload would be generally more useful.
Histogram will only give us aggregated values, which is fine, but we can also log all block sizes to have specific values.
There was a problem hiding this comment.
I can move it back to how it originally was then without logging the err.
pkg/compactor/block_upload.go
Outdated
|
|
||
| if blockSizeBytes > maxBlockSizeBytes || blockSizeBytes < 0 { | ||
| err := fmt.Errorf(maxBlockUploadSizeBytesFormat, maxBlockSizeBytes) | ||
| level.Error(logger).Log("err", err, "size", blockSizeBytes) |
There was a problem hiding this comment.
Agree that it seems unnecessary to hide computed block size from user.
pkg/compactor/block_upload.go
Outdated
| @@ -40,6 +42,10 @@ const ( | |||
| validationHeartbeatTimeout = 5 * time.Minute // Maximum duration of time to wait until a validation is able to be restarted | |||
| ) | |||
|
|
|||
| var maxBlockUploadSizeBytesFormat = globalerror.CompactorBlockUploadMaxBlockSizeBytes.MessageWithPerTenantLimitConfig( | |||
There was a problem hiding this comment.
No other error reported during block upload is currently using this way of reporting errors to user. I would suggest to tackle converting all block upload errors to the new system in a separate PR, and use "simple" way of error reporting in this PR.
There was a problem hiding this comment.
Changed to the simple way.
|
The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase |
andyasp
force-pushed
the
aasp/block-upload-max-block-size
branch
from
4988c4f
to
d8ea39a
Compare
andyasp
force-pushed
the
aasp/block-upload-max-block-size
branch
from
42816b5
to
45f5d58
Compare
What this PR does
Adds a limit for the maximum byte size for a block upload based off the block metadata. The sizes in the metadata are ensured to match the actual file sizes elsewhere already (during each file upload and during validation).
Which issue(s) this PR fixes or relates to
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]