[persist] refactor Blob impl for Azure for higher performance #31127
Conversation
ParkMyCar
changed the title
ParkMyCar
changed the title
* move fetching each chunk of a Part into a tokio::task * reduce copying in the case we get an invalid content-length header * add metrics for tracking the number of responses missing content-length
ParkMyCar
force-pushed
the
persist/azure-blob
branch
from
0f4186d to
8cc2778
Compare
ParkMyCar
changed the title
| // valuable. | ||
| let mut stream = blob.get().into_stream(); | ||
|
|
||
| while let Some(value) = stream.next().await { |
There was a problem hiding this comment.
Could we map/buffered here instead of spinning up individual tasks? A bit closer to the S3 impl (which does not fork off individual tasks) and makes it easier to cap the concurrency per fetch...
There was a problem hiding this comment.
good call! refactored to use FuturesOrdered like the S3 blob impl
src/persist/src/azure.rs
Outdated
| .lgbytes | ||
| .persist_azure | ||
| .new_region(usize::cast_from(content_length)); | ||
| PreSizedBuffer::Sized(region) | ||
| } | ||
| 0 => PreSizedBuffer::Unknown(Vec::new()), | ||
| 0 => { | ||
| metrics.get_invalid_resp.inc(); |
There was a problem hiding this comment.
The S3 metrics say that a "content-length of 0 isn't necessarily invalid", which makes sense to me. Could we inc this only if the size turns out to not match the header?
(Generally I'm not convinced of the need to have this defensive coding here, so it'd be handy if this metric fired only in the cases that it was actually load-bearing!)
There was a problem hiding this comment.
Ahhh yeah you're totally right, updated!
* remove metrics counting * use FuturesOrdered instead of tokio::task
…es not match content-length
This refactors the impl of
Blobfor Azure in a way that should be faster. TheBlobClientwe use from theazure_storage_blobcrate returns aStreamthat whenawait-ed sends a ranged GET request for a chunk of a blob. This PR refactors our impl so we await each ranged request in atokio::taskwhich increases the concurrency at which we fetch chunks of aPart.It also refactors how we handle the case when the
content-lengthheader is missing, and adds metrics so we can track how often this occurs.Motivation
Maybe progress against https://github.com/MaterializeInc/database-issues/issues/8892
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.