From 0baf90ee8882e18aaa226d42026975b12972622d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= Date: Tue, 16 Jun 2026 07:46:32 +0800 Subject: [PATCH] refactor: remove storage api facade (#3490) --- crates/ecstore/src/lib.rs | 1 - crates/ecstore/src/set_disk.rs | 5 +- crates/ecstore/src/sets.rs | 5 +- crates/ecstore/src/store.rs | 5 +- crates/ecstore/src/store_api/traits.rs | 10 --- .../ecstore/tests/storage_api_compat_test.rs | 15 ++-- docs/architecture/crate-boundaries.md | 19 +--- docs/architecture/migration-progress.md | 87 ++++++++++--------- scripts/check_architecture_migration_rules.sh | 29 +------ 9 files changed, 60 insertions(+), 116 deletions(-) diff --git a/crates/ecstore/src/lib.rs b/crates/ecstore/src/lib.rs index 151cee8b6..2cd3b5e6a 100644 --- a/crates/ecstore/src/lib.rs +++ b/crates/ecstore/src/lib.rs @@ -60,7 +60,6 @@ pub use global::{get_global_lock_client, get_global_lock_clients, set_global_loc pub use global::{new_object_layer_fn, resolve_object_store_handle, set_object_store_resolver}; pub use global::GLOBAL_Endpoints; -pub use store_api::StorageAPI; #[cfg(test)] mod rio_tests { diff --git a/crates/ecstore/src/set_disk.rs b/crates/ecstore/src/set_disk.rs index 675efd91f..6bfa18d16 100644 --- a/crates/ecstore/src/set_disk.rs +++ b/crates/ecstore/src/set_disk.rs @@ -55,7 +55,7 @@ use crate::{ BucketInfo, BucketOperations, BucketOptions, CompletePart, DeleteBucketOptions, DeletedObject, GetObjectReader, HTTPRangeSpec, HealOperations, ListMultipartsInfo, ListObjectsV2Info, ListOperations, MakeBucketOptions, MultipartInfo, MultipartOperations, MultipartUploadResult, NamespaceLocking, ObjectIO, ObjectInfo, ObjectOperations, PartInfo, - PutObjReader, StorageAPI, + PutObjReader, }, store_init::load_format_erasure, }; @@ -1633,9 +1633,6 @@ impl SetDisks { } } -#[async_trait::async_trait] -impl StorageAPI for SetDisks {} - #[async_trait::async_trait] impl NamespaceLocking for SetDisks { #[tracing::instrument(skip(self))] diff --git a/crates/ecstore/src/sets.rs b/crates/ecstore/src/sets.rs index 335c55847..95d1cdece 100644 --- a/crates/ecstore/src/sets.rs +++ b/crates/ecstore/src/sets.rs @@ -31,7 +31,7 @@ use crate::{ BucketInfo, BucketOperations, BucketOptions, CompletePart, DeleteBucketOptions, DeletedObject, GetObjectReader, HTTPRangeSpec, HealOperations, ListMultipartsInfo, ListObjectVersionsInfo, ListObjectsV2Info, ListOperations, MakeBucketOptions, MultipartInfo, MultipartOperations, MultipartUploadResult, NamespaceLocking, ObjectIO, ObjectInfo, - ObjectOperations, ObjectOptions, ObjectToDelete, PartInfo, PutObjReader, StorageAPI, + ObjectOperations, ObjectOptions, ObjectToDelete, PartInfo, PutObjReader, }, store_init::{check_format_erasure_values, get_format_erasure_in_quorum, load_format_erasure_all, save_format_file}, }; @@ -896,9 +896,6 @@ impl HealOperations for Sets { } } -#[async_trait::async_trait] -impl StorageAPI for Sets {} - #[async_trait::async_trait] impl NamespaceLocking for Sets { async fn new_ns_lock(&self, bucket: &str, object: &str) -> Result { diff --git a/crates/ecstore/src/store.rs b/crates/ecstore/src/store.rs index 132b71cd2..699dfe997 100644 --- a/crates/ecstore/src/store.rs +++ b/crates/ecstore/src/store.rs @@ -68,7 +68,7 @@ use crate::{ BucketInfo, BucketOperations, BucketOptions, CompletePart, DeleteBucketOptions, DeletedObject, GetObjectReader, HTTPRangeSpec, HealOperations, ListObjectsV2Info, ListOperations, MakeBucketOptions, MultipartOperations, MultipartUploadResult, NamespaceLocking, ObjectInfo, ObjectOperations, ObjectOptions, ObjectToDelete, PartInfo, - PutObjReader, StorageAPI, + PutObjReader, }, store_init, }; @@ -703,9 +703,6 @@ impl HealOperations for ECStore { } } -#[async_trait::async_trait] -impl StorageAPI for ECStore {} - #[async_trait::async_trait] impl NamespaceLocking for ECStore { async fn new_ns_lock(&self, bucket: &str, object: &str) -> Result { diff --git a/crates/ecstore/src/store_api/traits.rs b/crates/ecstore/src/store_api/traits.rs index 41213a878..4d3e5ca66 100644 --- a/crates/ecstore/src/store_api/traits.rs +++ b/crates/ecstore/src/store_api/traits.rs @@ -178,13 +178,3 @@ pub trait HealOperations: Send + Sync + Debug { pub trait NamespaceLocking: Send + Sync + Debug + 'static { async fn new_ns_lock(&self, bucket: &str, object: &str) -> Result; } - -/// Unified storage API combining all operation groups. -/// -/// Consumers can depend on specific sub-traits (e.g., `BucketOperations`) -/// when they don't need the full API surface. -#[allow(clippy::too_many_arguments)] -pub trait StorageAPI: - ObjectIO + BucketOperations + ObjectOperations + ListOperations + MultipartOperations + HealOperations + Debug -{ -} diff --git a/crates/ecstore/tests/storage_api_compat_test.rs b/crates/ecstore/tests/storage_api_compat_test.rs index ed56e4088..f311f2ce2 100644 --- a/crates/ecstore/tests/storage_api_compat_test.rs +++ b/crates/ecstore/tests/storage_api_compat_test.rs @@ -12,12 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use rustfs_ecstore::{ - disk::DiskStore, - error::Error, - store::ECStore, - store_api::{NamespaceLocking, StorageAPI}, -}; +use rustfs_ecstore::{disk::DiskStore, error::Error, store::ECStore, store_api::NamespaceLocking}; use rustfs_storage_api::StorageAdminApi; fn storage_admin_api_type_name() -> &'static str @@ -32,9 +27,9 @@ where std::any::type_name::() } -fn storage_api_with_namespace_locking_type_name() -> &'static str +fn namespace_locking_type_name() -> &'static str where - T: StorageAPI + NamespaceLocking, + T: NamespaceLocking, { std::any::type_name::() } @@ -45,6 +40,6 @@ fn ecstore_implements_storage_admin_api_contract() { } #[test] -fn ecstore_implements_storage_api_and_namespace_locking_contracts() { - assert!(storage_api_with_namespace_locking_type_name::().ends_with("::ECStore")); +fn ecstore_implements_namespace_locking_contract() { + assert!(namespace_locking_type_name::().ends_with("::ECStore")); } diff --git a/docs/architecture/crate-boundaries.md b/docs/architecture/crate-boundaries.md index 1e565ab2a..f039d20d0 100644 --- a/docs/architecture/crate-boundaries.md +++ b/docs/architecture/crate-boundaries.md @@ -85,8 +85,8 @@ the first extraction. ## Loss-Prevention Coverage -Architecture migration checks must keep public contract re-exports and storage -trait coverage from silently drifting during cleanup PRs. +Architecture migration checks must keep public contract re-exports and ECStore +compatibility coverage from silently drifting during cleanup PRs. Required `rustfs-storage-api` public re-exports: @@ -94,16 +94,5 @@ Required `rustfs-storage-api` public re-exports: - `pub use bucket::{BucketInfo, BucketOptions, DeleteBucketOptions, MakeBucketOptions, SRBucketDeleteOp};` - `pub use error::{StorageErrorCode, StorageResult};` -Required `StorageAPI` operation groups: - -- `ObjectIO` -- `BucketOperations` -- `ObjectOperations` -- `ListOperations` -- `MultipartOperations` -- `HealOperations` -- `Debug` - -`NamespaceLocking` must remain a separate operation group from the full -`StorageAPI` facade. ECStore must keep compile-time coverage for both -`StorageAdminApi` and `StorageAPI + NamespaceLocking`. +ECStore must keep compile-time coverage for both `StorageAdminApi` and the +separate `NamespaceLocking` operation group. diff --git a/docs/architecture/migration-progress.md b/docs/architecture/migration-progress.md index fb4819a72..3e8a099f1 100644 --- a/docs/architecture/migration-progress.md +++ b/docs/architecture/migration-progress.md @@ -5,16 +5,17 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block ## Current Context - Issue: [`rustfs/backlog#660`](https://github.com/rustfs/backlog/issues/660) -- Branch: `overtrue/arch-config-storage-boundary-cleanup` -- Baseline: `origin/main` at `fbab160c2b09075f5e2503a669d82917ef82d40e` +- Branch: `overtrue/arch-remove-storage-api-facade` +- Baseline: `origin/main` at `c26593fa7a4b55849e98832cde657c6d4b167262` - PR type for this branch: `consumer-migration` - Runtime behavior changes: no external behavior change expected. -- Rust code changes: remove stale full `StorageAPI` coupling from config - persistence tests, an unused S3 remove-client import, and an obsolete storage - list comment. -- CI/script changes: none. -- Docs changes: refresh the ECStore config persistence inventory and current - verification state. +- Rust code changes: remove the old unused `StorageAPI` facade, its ECStore + implementation blocks, its public re-export, and the stale compile-time + compatibility test coverage. +- CI/script changes: adjust architecture migration guardrails to keep the + remaining storage-admin and namespace-lock contracts covered. +- Docs changes: record the final old-facade cleanup slice and its verification + state. ## Phase 0 Tasks @@ -35,13 +36,12 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block - Completed slices: add a mechanical admin route matrix guard from [`admin-route-action-snapshot.md`](admin-route-action-snapshot.md) and `rustfs/src/admin/route_registration_test.rs`; add migration rules for - public storage-api re-export coverage, StorageAPI operation-group coverage, - NamespaceLocking separation, and ECStore compatibility-test coverage. + public storage-api re-export coverage and ECStore compatibility-test + coverage. - Acceptance: architecture migration rules fail if the public storage-api - contract re-export surface drifts, if `StorageAPI` stops covering the - documented storage operation groups, if `NamespaceLocking` is folded back - into the full storage facade, or if ECStore compile-time compatibility tests - for these contracts are removed. + contract re-export surface drifts or if ECStore compile-time compatibility + tests for the remaining storage-admin and namespace-lock contracts are + removed. - [x] `G-007` Create startup timeline table. - Acceptance: [`startup-timeline.md`](startup-timeline.md) records current binary startup order, side effects, fatal boundaries, and readiness stages. @@ -410,7 +410,9 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block - Completed slice: `rustfs/rustfs#3340` removed duplicate admin-read methods from the old `StorageAPI` trait and its ECStore/Sets/SetDisks/test implementations after API-007 migrated their consumers. - - Acceptance: old `StorageAPI` keeps storage operation traits while admin + - Final cleanup slice: remove the old `StorageAPI` facade after all real + consumers moved to concrete operation groups. + - Acceptance: storage operation traits remain available directly while admin inventory surfaces live only on `StorageAdminApi`. - [x] `API-009` Narrow metadata helper storage bounds. @@ -422,6 +424,8 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block - Cleanup slice: remove stale full `StorageAPI` dependencies from config persistence test support after the server-config persistence helpers moved to their actual object I/O and storage-admin bounds. + - Completed cleanup slice: `rustfs/rustfs#3489` removed the stale full + facade dependency from config persistence test support. - Acceptance: metadata helper contracts express the actual operation group they need, while callers and persistence behavior remain unchanged. @@ -431,8 +435,8 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block helper only needs `ObjectIO`. - Acceptance: resync metadata helpers express object-I/O-only persistence requirements, while replication execution, delete replication, multipart - replication, object lookups, and scheduling behavior remain on full - `StorageAPI` where needed. + replication, object lookups, and scheduling behavior remain on the concrete + operation groups they need. - [x] `API-011` Narrow scanner cache helper storage bounds. - Completed slice: `rustfs/rustfs#3348` narrowed scanner data-usage cache @@ -445,9 +449,9 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block cache paths, retry and timeout behavior, cache-save metrics, publish/update channel behavior, scanner cycle scheduling, disk scan concurrency, bucket scan semantics, lifecycle/replication decisions, and storage hot paths. - - Risk defense: do not move traits to `rustfs-storage-api`, do not remove - `StorageAPI`, do not alter helper bodies, and do not narrow scanner paths - that need bucket operations, disk inventory, or full storage orchestration. + - Risk defense: do not move traits to `rustfs-storage-api`, do not alter + helper bodies, and do not narrow scanner paths that need bucket operations, + disk inventory, or full storage orchestration. - Verification: focused compile/tests, migration guards, Rust risk scan, and required quality/architecture, migration-preservation, and testing/verification review passed. @@ -470,6 +474,8 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block resync leader-lock, delete replication, object replication, and multipart replication helpers away from full `StorageAPI` where they only need object I/O, object operations, list operations, and namespace locking. + - Final cleanup slice: remove the unused old `StorageAPI` facade, its + implementation blocks, public re-export, and stale guard coverage. - Acceptance: table catalog object backend contracts express the actual object read/write, metadata/delete, list, and namespace-lock capabilities they need; namespace-lock consumers depend on `NamespaceLocking` instead of @@ -480,8 +486,8 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block scanner/heal/replication/config persistence, and storage hot paths. - Risk defense: do not move traits into `rustfs-storage-api`, do not change lock implementation code, do not alter table catalog method bodies, and do - not retain stale API-012 compatibility markers after the old `StorageAPI` - lock method is removed. + not leave stale full-facade compatibility coverage after consumers move to + concrete operation groups. - Verification: focused compile/tests, migration guards, Rust risk scan, and required quality/architecture, migration-preservation, and testing/verification review passed. @@ -755,45 +761,44 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block ## Next PRs 1. `pure-move`/`consumer-migration`: continue larger cleanup slices with the - loss-prevention guards active for public re-exports and storage trait - coverage. + loss-prevention guards active for public re-exports and remaining storage + compatibility contracts. ## Pre-Push Review Log | Expert | Status | Notes | |---|---|---| -| Quality/architecture | passed | Config persistence tests now express their actual object I/O, namespace-lock, and storage-admin requirements instead of a full storage facade. | -| Migration preservation | passed | Config object encoding/decoding, metadata reads, namespace-lock behavior, and S3 remove-client behavior are unchanged. | -| Testing/verification | passed | Focused config tests, compile checks, migration/layer guards, formatting, diff hygiene, Rust risk scan, and full `make pre-commit` passed. | +| Quality/architecture | passed | Old `StorageAPI` facade removal leaves concrete operation traits and remaining storage-admin/namespace-lock contracts explicit. | +| Migration preservation | passed | ECStore/Sets/SetDisks operation implementations remain in place; only the unused aggregate facade and stale guard coverage are removed. | +| Testing/verification | passed | Focused compatibility test, compile checks, migration/layer guards, formatting, diff hygiene, Rust risk scan, and full `make pre-commit` passed. | ## Verification Notes -Passed on `fbab160c2b09075f5e2503a669d82917ef82d40e`: +Passed on `c26593fa7a4b55849e98832cde657c6d4b167262`: - `cargo check -p rustfs-ecstore`: passed. -- `cargo test -p rustfs-ecstore config::com --no-fail-fast`: passed. +- `cargo test -p rustfs-ecstore --test storage_api_compat_test --no-fail-fast`: + passed. - `cargo check -p rustfs -p rustfs-ecstore`: passed. - `./scripts/check_architecture_migration_rules.sh`: passed. - `./scripts/check_layer_dependencies.sh`: passed. - `cargo fmt --all --check`: passed. - `git diff --check`: passed. -- Rust risk scan: no new production `unwrap`/`expect`, lossy casts, string - errors, public boxed errors, production `println`/`eprintln`, or relaxed - atomics in added Rust lines. +- Rust risk scan: no new production `unwrap`/`expect`, panic/todo markers, + `unsafe`, or process-spawning calls in added Rust lines. - `make pre-commit`: passed. Notes: -- This slice removes test-only full facade scaffolding after config persistence - helpers already moved to narrower object I/O and storage-admin contracts. -- The S3 remove client had a stale full facade import only; behavior remains - unchanged. -- The slice does not remove the full storage facade or move traits across crate - boundaries. +- This slice removes the old full storage facade after no real code consumers + remain. +- The concrete storage operation traits, `StorageAdminApi`, and + `NamespaceLocking` remain available and covered. +- The slice does not move traits across crate boundaries. ## Handoff Notes -- Config storage boundary cleanup is locally verified and current with +- Old storage facade removal is locally verified on a branch current with `origin/main`. -- Remaining storage-facade cleanup can continue by migrating other consumers - that no longer need the full storage facade. +- After this lands, remaining storage work can focus on concrete operation + contracts instead of the aggregate facade. diff --git a/scripts/check_architecture_migration_rules.sh b/scripts/check_architecture_migration_rules.sh index 753e0e00b..934e4ff9f 100755 --- a/scripts/check_architecture_migration_rules.sh +++ b/scripts/check_architecture_migration_rules.sh @@ -49,7 +49,6 @@ PR_TYPE_HITS_FILE="${TMP_DIR}/pr_type_hits.txt" SOURCE_MARKERS_FILE="${TMP_DIR}/source_markers.txt" SOURCE_IDS_FILE="${TMP_DIR}/source_ids.txt" REGISTER_IDS_FILE="${TMP_DIR}/register_ids.txt" -STORAGE_API_SUPERTRAITS_FILE="${TMP_DIR}/storage_api_supertraits.txt" awk ' /^## PR Types$/ { @@ -187,30 +186,6 @@ require_source_line \ "pub use error::{StorageErrorCode, StorageResult};" \ "storage-api public error contract re-export" -perl -0ne ' - if (/pub trait StorageAPI:\s*(.*?)\s*\{\s*\}/s) { - my $body = $1; - while ($body =~ /\b([A-Z][A-Za-z0-9_]*)\b/g) { - print "$1\n"; - } - } -' "${ROOT_DIR}/crates/ecstore/src/store_api/traits.rs" | - sort -u >"$STORAGE_API_SUPERTRAITS_FILE" - -if [[ ! -s "$STORAGE_API_SUPERTRAITS_FILE" ]]; then - report_failure "StorageAPI supertrait declaration not found in crates/ecstore/src/store_api/traits.rs" -fi - -for required_trait in ObjectIO BucketOperations ObjectOperations ListOperations MultipartOperations HealOperations Debug; do - if ! contains_line "$required_trait" "$STORAGE_API_SUPERTRAITS_FILE"; then - report_failure "StorageAPI no longer covers required operation group ${required_trait}" - fi -done - -if contains_line "NamespaceLocking" "$STORAGE_API_SUPERTRAITS_FILE"; then - report_failure "NamespaceLocking must remain separate from the full StorageAPI facade" -fi - require_source_contains \ "crates/ecstore/src/store_api/traits.rs" \ "pub trait NamespaceLocking: Send + Sync + Debug + 'static" \ @@ -221,8 +196,8 @@ require_source_contains \ "ECStore StorageAdminApi compile-time coverage test" require_source_contains \ "crates/ecstore/tests/storage_api_compat_test.rs" \ - "fn ecstore_implements_storage_api_and_namespace_locking_contracts()" \ - "ECStore StorageAPI and NamespaceLocking compile-time coverage test" + "fn ecstore_implements_namespace_locking_contract()" \ + "ECStore NamespaceLocking compile-time coverage test" if (( FAILURES > 0 )); then exit 1