Sruhvx-jpg commented on PR #3165:
URL: https://github.com/apache/iceberg-rust/pull/3165#issuecomment-5792328243
@laskoviymishka Addressed all the feedback from this pass.
Regarding the multipart upload lifecycle, we replaced `WriteMultipart` with
direct `Box<dyn MultipartUpload>` management so `abort()` can be awaited if
completion or part uploads fail:
```rust
struct ObjectStoreWriter {
upload: Option<Box<dyn MultipartUpload>>,
buffer: BytesMut,
tasks: Vec<UploadPart>,
}
```
In `close()`, any failure during in-flight joins or
`upload.complete().await` explicitly awaits `upload.abort().await` before
returning `Err`, while `Drop` remains as the background safety net. We also
added two extra hardening guards here: (1) handling 0-byte uploads so closing a
fresh writer flushes an empty part (satisfying S3's requirement of $\ge$ 1 part
to complete a multipart upload), and (2) guarding `append_bytes` against
`remaining == 0` edge cases. Let me know if you are good with these extra
guards.
For testing abort behavior, we dropped the false-positive integration
assertion and introduced unit tests backed by a `MockMultipartUpload` spy:
```rust
#[tokio::test]
async fn test_writer_part_failure_aborts() {
let (mut writer, _parts, completed, aborted) = make_mock_writer(true,
false);
writer.write(Bytes::from(vec![0u8; 6 * 1024 * 1024])).await.unwrap();
let err = writer.close().await.unwrap_err();
assert!(aborted.load(Ordering::SeqCst));
}
```
This deterministically verifies the atomic `aborted` flag across clean
closes, part failures, complete failures, and unclosed drops.
On error mapping parity, we aligned storage errors to
`ErrorKind::Unexpected` to match opendal and Iceberg conventions:
```rust
object_store::Error::NotFound { path, .. } => (ErrorKind::Unexpected,
format!("Object not found: {path}")),
object_store::Error::AlreadyExists { path, .. } => (ErrorKind::Unexpected,
format!("Object already exists: {path}")),
object_store::Error::PermissionDenied { path, .. } =>
(ErrorKind::Unexpected, format!("Permission denied: {path}")),
object_store::Error::Unauthenticated { path, .. } => (ErrorKind::Unexpected,
format!("Unauthenticated: {path}")),
```
We also mapped closed writer operations to `ErrorKind::PreconditionFailed`,
unsupported schemes to `ErrorKind::FeatureUnsupported`, and config key parsing
to `ErrorKind::DataInvalid`.
For `delete_stream`, `StoreAndPath` now returns `bucket: String` directly,
eliminating the duplicate `parse_s3_url` call, and grouping is handled via a
single `BucketBatch` map:
```rust
struct BucketBatch {
store: Arc<dyn ObjectStore>,
locations: Vec<ObjectStorePath>,
}
```
This removes the two-map structure and eliminates the `.expect("store must
exist")` panic entirely.
Lastly, for URL delimiter parsing, we switched from
`trim_start_matches('/')` to `strip_prefix`:
```rust
let relative = url.path().strip_prefix('/').unwrap_or(url.path());
```
This preserves consecutive slashes on object keys (`s3://bucket//path` ->
`/path`).
All 34 unit tests, MinIO integration tests, and clippy/formatting checks are
passing cleanly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
