Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove use of ObjectInfo in S3 client HeadObject response #1058

Merged
merged 2 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
13 changes: 13 additions & 0 deletions mountpoint-s3-client/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,18 @@
## Unreleased

### Other changes

* No other changes.

### Breaking changes

* `HeadObjectResult` no longer contains an `ObjectInfo` struct.
Instead, it returns the object attributes as individual fields on the `HeadObjectResult`.
The entity tag field has also changed and is now of type `ETag` rather than `String`.
([#1058](https://github.com/awslabs/mountpoint-s3/pull/1058))
* `HeadObjectResult` no longer provides the bucket and key used in the original request.
([#1058](https://github.com/awslabs/mountpoint-s3/pull/1058))

## v0.11.0 (October 17, 2024)

### Breaking changes
Expand Down
16 changes: 6 additions & 10 deletions mountpoint-s3-client/src/mock_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -687,15 +687,11 @@ impl ObjectClient for MockClient {
let objects = self.objects.read().unwrap();
if let Some(object) = objects.get(key) {
Ok(HeadObjectResult {
bucket: bucket.to_string(),
object: ObjectInfo {
key: key.to_string(),
size: object.size as u64,
last_modified: object.last_modified,
etag: object.etag.as_str().to_string(),
storage_class: object.storage_class.clone(),
restore_status: object.restore_status,
},
size: object.size as u64,
last_modified: object.last_modified,
etag: object.etag.clone(),
storage_class: object.storage_class.clone(),
restore_status: object.restore_status,
})
} else {
Err(ObjectClientError::ServiceError(HeadObjectError::NotFound))
Expand Down Expand Up @@ -1681,7 +1677,7 @@ mod tests {

// head_object returns storage class
let head_result = client.head_object(bucket, key).await.unwrap();
assert_eq!(head_result.object.storage_class.as_deref(), storage_class);
assert_eq!(head_result.storage_class.as_deref(), storage_class);

// list_objects returns storage class
let list_result = client.list_objects(bucket, None, "/", 1, "").await.unwrap();
Expand Down
25 changes: 21 additions & 4 deletions mountpoint-s3-client/src/object_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,11 +210,28 @@ pub enum ListObjectsError {
#[derive(Debug)]
#[non_exhaustive]
pub struct HeadObjectResult {
/// The name of the bcuket
pub bucket: String,
/// Size of the object in bytes.
///
/// Refers to the `Content-Length` HTTP header for HeadObject.
pub size: u64,

/// The time this object was last modified.
pub last_modified: OffsetDateTime,

/// Entity tag of this object.
pub etag: ETag,

/// Storage class for this object.
///
/// The value is optional because HeadObject does not return the storage class in its response
/// for objects in the S3 Standard storage class.
/// See examples in the
/// [Amazon S3 API Reference](https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadObject.html#API_HeadObject_Examples).
pub storage_class: Option<String>,

/// Object metadata
pub object: ObjectInfo,
/// Objects in flexible retrieval storage classes (such as GLACIER and DEEP_ARCHIVE) are only
/// accessible after restoration
pub restore_status: Option<RestoreStatus>,
}

/// Errors returned by a [`head_object`](ObjectClient::head_object) request
Expand Down
20 changes: 7 additions & 13 deletions mountpoint-s3-client/src/s3_crt_client/head_object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,7 @@ use time::format_description::well_known::Rfc2822;
use time::OffsetDateTime;
use tracing::error;

use crate::object_client::{
HeadObjectError, HeadObjectResult, ObjectClientError, ObjectClientResult, ObjectInfo, RestoreStatus,
};
use crate::object_client::{HeadObjectError, HeadObjectResult, ObjectClientError, ObjectClientResult, RestoreStatus};
use crate::s3_crt_client::{S3CrtClient, S3Operation, S3RequestError};

#[derive(Error, Debug)]
Expand Down Expand Up @@ -85,23 +83,23 @@ impl HeadObjectResult {
Ok(Some(RestoreStatus::Restored { expiry: expiry.into() }))
}

fn parse_from_hdr(bucket: String, key: String, headers: &Headers) -> Result<Self, ParseError> {
/// Parse from HeadObject headers
fn parse_from_hdr(headers: &Headers) -> Result<Self, ParseError> {
let last_modified = OffsetDateTime::parse(&get_field(headers, "Last-Modified")?, &Rfc2822)
.map_err(|e| ParseError::OffsetDateTime(e, "LastModified".into()))?;
let size = u64::from_str(&get_field(headers, "Content-Length")?)
.map_err(|e| ParseError::Int(e, "ContentLength".into()))?;
let etag = get_field(headers, "Etag")?;
let storage_class = get_optional_field(headers, "x-amz-storage-class")?;
let restore_status = Self::parse_restore_status(headers)?;
let object = ObjectInfo {
key,
let result = HeadObjectResult {
size,
last_modified,
storage_class,
restore_status,
etag,
etag: etag.into(),
};
Ok(HeadObjectResult { bucket, object })
Ok(result)
}
}

Expand Down Expand Up @@ -137,11 +135,7 @@ impl S3CrtClient {
span,
move |headers, _status| {
let mut header = header1.lock().unwrap();
*header = Some(HeadObjectResult::parse_from_hdr(
bucket.to_string(),
key.to_string(),
headers,
));
*header = Some(HeadObjectResult::parse_from_hdr(headers));
},
|_, _| (),
move |result| {
Expand Down
36 changes: 15 additions & 21 deletions mountpoint-s3-client/tests/head_object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ async fn test_head_object() {
let sdk_client = get_test_sdk_client().await;
let (bucket, prefix) = get_test_bucket_and_prefix("test_head_object");

let key = format!("{prefix}/hello");
let key = format!("{prefix}hello");
let body = b"hello world!";
sdk_client
.put_object()
Expand All @@ -36,9 +36,11 @@ async fn test_head_object() {
let client: S3CrtClient = get_test_client();
let result = client.head_object(&bucket, &key).await.expect("head_object failed");

assert_eq!(result.bucket, bucket);
assert_eq!(result.object.key, key);
assert_eq!(result.object.size as usize, body.len());
assert_eq!(
result.size as usize,
body.len(),
"HeadObject reported size should match uploaded body length",
);
}

#[test_case("INTELLIGENT_TIERING")]
Expand All @@ -65,11 +67,9 @@ async fn test_head_object_storage_class(storage_class: &str) {
let client: S3CrtClient = get_test_client();
let result = client.head_object(&bucket, &key).await.expect("head_object failed");

assert_eq!(result.bucket, bucket);
assert_eq!(result.object.key, key);
assert_eq!(result.object.size as usize, body.len());
assert_eq!(result.object.storage_class.as_deref(), Some(storage_class));
assert!(result.object.restore_status.is_none());
assert_eq!(result.size as usize, body.len());
assert_eq!(result.storage_class.as_deref(), Some(storage_class));
assert!(result.restore_status.is_none());
}

#[tokio::test]
Expand Down Expand Up @@ -140,10 +140,8 @@ async fn test_head_object_restored() {
let client: S3CrtClient = get_test_client();
let result = client.head_object(&bucket, &key).await.expect("head_object failed");

assert_eq!(result.bucket, bucket);
assert_eq!(result.object.key, key);
assert!(
result.object.restore_status.is_none(),
result.restore_status.is_none(),
"object should become restored only after restoration"
);

Expand All @@ -168,18 +166,14 @@ async fn test_head_object_restored() {

let timeout = Duration::from_secs(300);
let start = Instant::now();
let mut timeouted = true;
let mut timeout_exceeded = true;
while start.elapsed() < timeout {
let object = client
.head_object(&bucket, &key)
.await
.expect("head_object failed")
.object;
if let Some(RestoreStatus::Restored { expiry: _ }) = object.restore_status {
timeouted = false;
let response = client.head_object(&bucket, &key).await.expect("head_object failed");
if let Some(RestoreStatus::Restored { expiry: _ }) = response.restore_status {
timeout_exceeded = false;
break;
}
std::thread::sleep(Duration::from_secs(1));
}
assert!(!timeouted, "timeouted while waiting for object become restored");
assert!(!timeout_exceeded, "timeouted while waiting for object become restored");
}
6 changes: 2 additions & 4 deletions mountpoint-s3/examples/prefetch_benchmark.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use std::str::FromStr;
use std::sync::atomic::{AtomicU64, Ordering};
use std::sync::Arc;
use std::thread;
Expand All @@ -10,7 +9,6 @@ use mountpoint_s3::mem_limiter::MemoryLimiter;
use mountpoint_s3::object::ObjectId;
use mountpoint_s3::prefetch::{default_prefetch, Prefetch, PrefetchResult};
use mountpoint_s3_client::config::{EndpointConfig, S3ClientConfig};
use mountpoint_s3_client::types::ETag;
use mountpoint_s3_client::{ObjectClient, S3CrtClient};
use mountpoint_s3_crt::common::rust_log_adapter::RustLogAdapter;
use sysinfo::{RefreshKind, System};
Expand Down Expand Up @@ -113,8 +111,8 @@ fn main() {
let mem_limiter = Arc::new(MemoryLimiter::new(client.clone(), max_memory_target));

let head_object_result = block_on(client.head_object(bucket, key)).expect("HeadObject failed");
let size = head_object_result.object.size;
let etag = ETag::from_str(&head_object_result.object.etag).unwrap();
let size = head_object_result.size;
let etag = head_object_result.etag;

for i in 0..iterations.unwrap_or(1) {
let runtime = client.event_loop_group();
Expand Down
5 changes: 2 additions & 3 deletions mountpoint-s3/src/superblock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -699,8 +699,8 @@ impl SuperblockInner {
select_biased! {
result = file_lookup => {
match result {
Ok(HeadObjectResult { object, .. }) => {
let stat = InodeStat::for_file(object.size as usize, object.last_modified, Some(object.etag.clone()), object.storage_class, object.restore_status, self.config.cache_config.file_ttl);
Ok(HeadObjectResult { size, last_modified, restore_status ,etag, storage_class, .. }) => {
let stat = InodeStat::for_file(size as usize, last_modified, Some(etag.as_str().to_string()), storage_class, restore_status, self.config.cache_config.file_ttl);
file_state = Some(stat);
}
// If the object is not found, might be a directory, so keep going
Expand Down Expand Up @@ -1276,7 +1276,6 @@ mod tests {
.head_object(bucket, file.inode.full_key())
.await
.expect("object should exist")
.object
.last_modified;
assert_inode_stat!(file, InodeKind::File, modified_time, object_size);
assert_eq!(
Expand Down
Loading