laskoviymishka commented on code in PR #3288: URL: https://github.com/apache/iceberg-rust/pull/3288#discussion_r4204862044
########## crates/storage/common/Cargo.toml: ########## @@ -0,0 +1,53 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +[package] +edition = { workspace = true } +homepage = { workspace = true } +license = { workspace = true } +name = "iceberg-storage-common" +repository = { workspace = true } +rust-version = { workspace = true } +version = { workspace = true } + +categories = ["database"] +description = "Apache Iceberg Storage Common Test Suite" +keywords = ["iceberg", "storage"] +publish = false + +[dependencies] +bytes = { workspace = true } +futures = { workspace = true } +iceberg = { workspace = true } +iceberg_test_utils = { path = "../../test_utils", features = ["tests"] } +reqwest = { workspace = true } +tempfile = { workspace = true } +tokio = { workspace = true, features = [ Review Comment: `src/` only needs `rt` and `time` here (`tokio::spawn` plus `sleep`); `io-util`, `macros`, and `net` are only pulled in by the test binaries, so under `[dependencies]` they get turned on for every consumer of the crate. I'd move those three down: ```toml [dependencies] tokio = { workspace = true, features = ["rt", "time"] } [dev-dependencies] tokio = { workspace = true, features = ["io-util", "macros", "net", "rt-multi-thread"] } ``` that's the last of the two Cargo.toml nits I left open. ########## Cargo.toml: ########## @@ -115,6 +115,7 @@ pyo3 = "0.29" quote = "1" rand = "0.9.3" regex = "1.11.3" +reqsign-core = "3.0.0" Review Comment: now that `reqsign-core` is a workspace dep, the opendal crate still pins it as a literal `"3.0.0"` (`crates/storage/opendal/Cargo.toml:58`) — point that at `reqsign-core = { workspace = true, optional = true }` so the version lives in one place. ########## crates/storage/common/src/endpoint_probe.rs: ########## @@ -0,0 +1,97 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Fast endpoint probe and readiness utilities for integration test emulators. + +use std::time::Duration; + +use iceberg::io::FileIO; +use tokio::time::sleep; + +use crate::harness::StorageHarness; + +const DEFAULT_PROBE_TIMEOUT_MS: u64 = 1000; +const DEFAULT_PROBE_RETRIES: usize = 3; +const PROBE_RETRY_INTERVAL_MS: u64 = 200; + +fn get_probe_timeout() -> Duration { + let ms = std::env::var("ICEBERG_PROBE_TIMEOUT_MS") + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(DEFAULT_PROBE_TIMEOUT_MS); + Duration::from_millis(ms) +} + +/// Fast probe to check if an endpoint service is listening before entering retry loops. +/// +/// Note: Any HTTP response from `.send().await.is_ok()` (including 4xx/5xx) is treated +/// as reachable, as it proves the underlying server is up, listening on the port, +/// and actively responding to HTTP requests. +pub async fn is_endpoint_reachable(endpoint: &str) -> bool { + let Ok(client) = reqwest::Client::builder() Review Comment: `reqwest::Client::builder()` honors `HTTP_PROXY`/`ALL_PROXY`, so on a runner with a proxy a GET to `localhost:9000` can come back as a proxy 502 — and since any HTTP response counts as reachable, the probe returns `true` for a down endpoint and the `ICEBERG_REQUIRE_STORAGE` hard-fail never fires (the failure just moves to `wait_until_ready` or a test body). one `.no_proxy()` on the builder closes it. ########## crates/storage/common/tests/resolving_suite.rs: ########## @@ -0,0 +1,344 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! OpenDAL resolving storage integration tests. + +mod common; + +use std::sync::Arc; + +use bytes::Bytes; +use common::{StorageKind, load_storage, unique_path}; +use iceberg::io::{FileIOBuilder, S3_ENDPOINT, S3_PATH_STYLE_ACCESS, S3_REGION}; +use iceberg_storage_common::roundtrip_file_io; +use iceberg_storage_opendal::{ + AwsCredential, CustomAwsCredentialLoader, OpenDalResolvingStorageFactory, ProvideCredential, +}; +use iceberg_test_utils::{get_object_store_endpoint, set_up}; +use reqsign_core::Context; +use rstest::rstest; +use tempfile::TempDir; + +#[rstest] +#[case::opendal_resolving(StorageKind::OpenDalResolving)] +#[tokio::test] +async fn test_mixed_scheme_write_and_read(#[case] kind: StorageKind) -> iceberg::Result<()> { + let Some(harness) = load_storage(kind).await else { + return Ok(()); + }; + + let s3_path = unique_path(&harness, "test_mixed_scheme_write_and_read"); + let temp_dir = TempDir::new().unwrap(); Review Comment: the four `TempDir::new().unwrap()`s (45, 98, 141, 199) can go to `?` via `map_err` the way we did in `file_io.rs`, and `test_resolving_serialization_roundtrip` (325) is now a byte-for-byte copy of `run_file_io_serialization_roundtrip` — just call `run_file_io_serialization_roundtrip(&harness).await`. non-blocking, same cleanup we noted last round. -- 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]
