Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions src/aws/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ use url::Url;
/// Default metadata endpoint
static DEFAULT_METADATA_ENDPOINT: &str = "http://169.254.169.254";

/// AWS S3 does not support copy operations larger than 5 GiB in a single request. See
/// [CopyObject](https://docs.aws.amazon.com/AmazonS3/latest/userguide/copy-object.html) for more
/// details.
const MAX_SINGLE_REQUEST_COPY_SIZE: u64 = 5 * 1024 * 1024 * 1024;

/// A specialized `Error` for object store-related errors
#[derive(Debug, thiserror::Error)]
enum Error {
Expand Down Expand Up @@ -189,6 +194,10 @@ pub struct AmazonS3Builder {
request_payer: ConfigValue<bool>,
/// The [`HttpConnector`] to use
http_connector: Option<Arc<dyn HttpConnector>>,
/// Threshold (bytes) above which copy uses multipart copy. If not set, defaults to 5 GiB.
multipart_copy_threshold: Option<ConfigValue<u64>>,
/// Preferred multipart copy part size (bytes). If not set, defaults to 5 GiB.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a question I have but that I also think should be included in the doc-string: if the object was created using a multi-part upload, are there any requirements on the alignment of the copy-parts or is this irrelevant?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not explicitly documented (though there is a note around range requests). There's every chance it's faster to line up the part boundaries exactly, but I haven't tested this directly.

I hadn't thought about it and this bothers me. It doesn't feel good to be opaquely putting part boundaries into a copied object that the user has no visibility into. This also points to the right API being one where the user creates parts directly and keeps track of the boundaries.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW #121

multipart_copy_part_size: Option<ConfigValue<u64>>,
}

/// Configuration keys for [`AmazonS3Builder`]
Expand Down Expand Up @@ -423,6 +432,10 @@ pub enum AmazonS3ConfigKey {

/// Encryption options
Encryption(S3EncryptionConfigKey),
/// Threshold (bytes) to switch to multipart copy
MultipartCopyThreshold,
/// Preferred multipart copy part size (bytes)
MultipartCopyPartSize,
}

impl AsRef<str> for AmazonS3ConfigKey {
Expand Down Expand Up @@ -455,6 +468,8 @@ impl AsRef<str> for AmazonS3ConfigKey {
Self::RequestPayer => "aws_request_payer",
Self::Client(opt) => opt.as_ref(),
Self::Encryption(opt) => opt.as_ref(),
Self::MultipartCopyThreshold => "aws_multipart_copy_threshold",
Self::MultipartCopyPartSize => "aws_multipart_copy_part_size",
}
}
}
Expand Down Expand Up @@ -499,6 +514,12 @@ impl FromStr for AmazonS3ConfigKey {
"aws_conditional_put" | "conditional_put" => Ok(Self::ConditionalPut),
"aws_disable_tagging" | "disable_tagging" => Ok(Self::DisableTagging),
"aws_request_payer" | "request_payer" => Ok(Self::RequestPayer),
"aws_multipart_copy_threshold" | "multipart_copy_threshold" => {
Ok(Self::MultipartCopyThreshold)
}
"aws_multipart_copy_part_size" | "multipart_copy_part_size" => {
Ok(Self::MultipartCopyPartSize)
}
// Backwards compatibility
"aws_allow_http" => Ok(Self::Client(ClientConfigKey::AllowHttp)),
"aws_server_side_encryption" | "server_side_encryption" => Ok(Self::Encryption(
Expand Down Expand Up @@ -666,6 +687,12 @@ impl AmazonS3Builder {
self.encryption_customer_key_base64 = Some(value.into())
}
},
AmazonS3ConfigKey::MultipartCopyThreshold => {
self.multipart_copy_threshold = Some(ConfigValue::Deferred(value.into()))
}
AmazonS3ConfigKey::MultipartCopyPartSize => {
self.multipart_copy_part_size = Some(ConfigValue::Deferred(value.into()))
}
};
self
}
Expand Down Expand Up @@ -733,6 +760,14 @@ impl AmazonS3Builder {
self.encryption_customer_key_base64.clone()
}
},
AmazonS3ConfigKey::MultipartCopyThreshold => self
.multipart_copy_threshold
.as_ref()
.map(|x| x.to_string()),
AmazonS3ConfigKey::MultipartCopyPartSize => self
.multipart_copy_part_size
.as_ref()
.map(|x| x.to_string()),
}
}

Expand Down Expand Up @@ -1029,6 +1064,18 @@ impl AmazonS3Builder {
self
}

/// Set threshold (bytes) above which copy uses multipart copy
pub fn with_multipart_copy_threshold(mut self, threshold_bytes: u64) -> Self {
self.multipart_copy_threshold = Some(ConfigValue::Parsed(threshold_bytes));
self
}

/// Set preferred multipart copy part size (bytes)
pub fn with_multipart_copy_part_size(mut self, part_size_bytes: u64) -> Self {
self.multipart_copy_part_size = Some(ConfigValue::Parsed(part_size_bytes));
self
}

/// Create a [`AmazonS3`] instance from the provided values,
/// consuming `self`.
pub fn build(mut self) -> Result<AmazonS3> {
Expand Down Expand Up @@ -1185,6 +1232,17 @@ impl AmazonS3Builder {
S3EncryptionHeaders::default()
};

let multipart_copy_threshold = self
.multipart_copy_threshold
.map(|val| val.get())
.transpose()?
.unwrap_or(MAX_SINGLE_REQUEST_COPY_SIZE);
let multipart_copy_part_size = self
.multipart_copy_part_size
.map(|val| val.get())
.transpose()?
.unwrap_or(MAX_SINGLE_REQUEST_COPY_SIZE);

let config = S3Config {
region,
bucket,
Expand All @@ -1201,6 +1259,8 @@ impl AmazonS3Builder {
conditional_put: self.conditional_put.get()?,
encryption_headers,
request_payer: self.request_payer.get()?,
multipart_copy_threshold,
multipart_copy_part_size,
};

let http_client = http.connect(&config.client_options)?;
Expand Down
24 changes: 23 additions & 1 deletion src/aws/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ impl From<Error> for crate::Error {
pub(crate) enum PutPartPayload<'a> {
Part(PutPayload),
Copy(&'a Path),
CopyRange(&'a Path, std::ops::Range<u64>),
}

impl Default for PutPartPayload<'_> {
Expand Down Expand Up @@ -209,6 +210,10 @@ pub(crate) struct S3Config {
pub conditional_put: S3ConditionalPut,
pub request_payer: bool,
pub(super) encryption_headers: S3EncryptionHeaders,
/// Threshold in bytes above which copy will use multipart copy
pub multipart_copy_threshold: u64,
/// Preferred multipart copy part size in bytes (None => auto)
pub multipart_copy_part_size: u64,
}

impl S3Config {
Expand Down Expand Up @@ -681,7 +686,10 @@ impl S3Client {
part_idx: usize,
data: PutPartPayload<'_>,
) -> Result<PartId> {
let is_copy = matches!(data, PutPartPayload::Copy(_));
let is_copy = matches!(
data,
PutPartPayload::Copy(_) | PutPartPayload::CopyRange(_, _)
);
let part = (part_idx + 1).to_string();

let mut request = self
Expand All @@ -695,6 +703,18 @@ impl S3Client {
"x-amz-copy-source",
&format!("{}/{}", self.config.bucket, encode_path(path)),
),
PutPartPayload::CopyRange(path, range) => {
// AWS expects inclusive end for copy range header
let start = range.start;
let end_inclusive = range.end.saturating_sub(1);
let range_value = format!("bytes={}-{}", start, end_inclusive);
request
.header(
"x-amz-copy-source",
&format!("{}/{}", self.config.bucket, encode_path(path)),
)
.header("x-amz-copy-source-range", &range_value)
}
};

if self
Expand Down Expand Up @@ -1000,6 +1020,8 @@ mod tests {
conditional_put: Default::default(),
encryption_headers: Default::default(),
request_payer: false,
multipart_copy_threshold: 5 * 1024 * 1024 * 1024,
multipart_copy_part_size: 5 * 1024 * 1024 * 1024,
};

let client = S3Client::new(config, HttpClient::new(reqwest::Client::new()));
Expand Down
132 changes: 89 additions & 43 deletions src/aws/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,64 @@ impl AmazonS3 {
fn path_url(&self, path: &Path) -> String {
self.client.config.path_url(path)
}

/// Perform a multipart copy operation
///
/// If the multipart upload fails, this function makes a best effort attempt to clean it up.
/// It's the caller's responsibility to add a lifecycle rule if guaranteed cleanup is required,
/// as we cannot protect against an ill-timed process crash.
async fn copy_multipart(
&self,
from: &Path,
to: &Path,
size: u64,
mode: CompleteMultipartMode,
) -> Result<()> {
// Perform multipart copy using UploadPartCopy
let upload_id = self
.client
.create_multipart(to, PutMultipartOptions::default())
.await?;

// S3 requires minimum 5 MiB per part (except final) and max 10,000 parts
let part_size = self.client.config.multipart_copy_part_size;

let mut parts = Vec::new();
let mut offset: u64 = 0;
let mut idx: usize = 0;
while offset < size {
let end = if size - offset <= part_size {
size
} else {
offset + part_size
};
let payload = if offset == 0 && end == size {
PutPartPayload::Copy(from)
} else {
PutPartPayload::CopyRange(from, offset..end)
};
let part = match self.client.put_part(to, &upload_id, idx, payload).await {
Ok(part) => part,
Err(e) => {
let _ = self.client.abort_multipart(to, &upload_id).await;
return Err(e);
}
};
parts.push(part);
idx += 1;
offset = end;
}
let res = self
.client
.complete_multipart(to, &upload_id, parts, mode)
.await
.map(|_| ());

if res.is_err() {
let _ = self.client.abort_multipart(to, &upload_id).await;
}
res
}
}

#[async_trait]
Expand Down Expand Up @@ -305,14 +363,31 @@ impl ObjectStore for AmazonS3 {
mode,
extensions: _,
} = options;
// Determine source size to decide between single CopyObject and multipart copy

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some way we can avoid this, e.g. we try CopyObject normally and on error fallback to multipart? Otherwise this adds an additional S3 roundtrip to every copy request.

let head_meta = self
.client
.get_opts(
from,
GetOptions {
head: true,
..Default::default()
},
)
.await?
.meta;

match mode {
CopyMode::Overwrite => {
self.client
.copy_request(from, to)
.idempotent(true)
.send()
.await?;
if head_meta.size <= self.client.config.multipart_copy_threshold {
self.client
.copy_request(from, to)
.idempotent(true)
.send()
.await?;
} else {
self.copy_multipart(from, to, head_meta.size, CompleteMultipartMode::Overwrite)
.await?;
}
Ok(())
}
CopyMode::Create => {
Expand All @@ -322,45 +397,16 @@ impl ObjectStore for AmazonS3 {
}
Some(S3CopyIfNotExists::HeaderWithStatus(k, v, status)) => (k, v, *status),
Some(S3CopyIfNotExists::Multipart) => {
let upload_id = self
.client
.create_multipart(to, PutMultipartOptions::default())
.await?;

let res = async {
let part_id = self
.client
.put_part(to, &upload_id, 0, PutPartPayload::Copy(from))
.await?;
match self
.client
.complete_multipart(
to,
&upload_id,
vec![part_id],
CompleteMultipartMode::Create,
)
.await
{
Err(e @ Error::Precondition { .. }) => Err(Error::AlreadyExists {
return self
.copy_multipart(from, to, head_meta.size, CompleteMultipartMode::Create)
.await
.map_err(|err| match err {
Error::Precondition { .. } => Error::AlreadyExists {
path: to.to_string(),
source: Box::new(e),
}),
Ok(_) => Ok(()),
Err(e) => Err(e),
}
}
.await;

// If the multipart upload failed, make a best effort attempt to
// clean it up. It's the caller's responsibility to add a
// lifecycle rule if guaranteed cleanup is required, as we
// cannot protect against an ill-timed process crash.
if res.is_err() {
let _ = self.client.abort_multipart(to, &upload_id).await;
}

return res;
source: Box::new(err),
},
other => other,
});
}
None => {
return Err(Error::NotSupported {
Expand Down
9 changes: 9 additions & 0 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,15 @@ impl Parse for u32 {
}
}

impl Parse for u64 {
fn parse(v: &str) -> Result<Self> {
Self::from_str(v).map_err(|_| Error::Generic {
store: "Config",
source: format!("failed to parse \"{v}\" as u64").into(),
})
}
}

impl Parse for HeaderValue {
fn parse(v: &str) -> Result<Self> {
Self::from_str(v).map_err(|_| Error::Generic {
Expand Down