Code Review

This PR introduces the StorageOptionsAccessor abstraction to unify storage options and provider management for distributed engine scenarios. The refactoring consolidates storage_options and storage_options_provider into a single accessor pattern with caching and automatic credential refresh.

P0 Issues

None identified.

P1 Issues

1. Potential race condition in credential refresh retry loop (storage_options.rs)

   In get_provider_options_with_version(), when the write lock is busy, the code returns None and the caller retries with a 10ms sleep. However, there's no maximum retry limit, which could lead to indefinite spinning in pathological cases:

   loop {
    match self.do_get_provider_options().await? {
    Some((opts, version)) => return Ok((opts, version)),
    None => {
    tokio::time::sleep(Duration::from_millis(10)).await;
    }
    }
   }

   Consider adding a maximum retry count or exponential backoff with a timeout.

2. Test coverage for StorageOptionsAccessor

   The new StorageOptionsAccessor struct (~300+ lines of new code) appears to lack dedicated unit tests. Given this is a critical component for credential management across Python, Java, and Rust, I'd recommend adding tests covering:

   • Caching behavior (expiration, refresh before expiry)
   • Merging of initial options with provider options
   • Version tracking on refresh
   • Edge cases (empty options, no provider, etc.)

Observations

• The API surface is well-designed with clear precedence rules (provider options override initial options).
• The migration from storage_options + storage_options_provider to unified storage_options_accessor is consistently applied across Python, Java, and Rust bindings.
• Documentation and examples in the Java/Python bindings are thorough.

Code Review

This PR refactors how storage options and credential providers are handled by introducing a unified StorageOptionsAccessor that manages both static options and dynamic providers with automatic caching and refresh.

Summary

The changes unify the previous separate storage_options and storage_options_provider into a single StorageOptionsAccessor that handles option merging, caching, and credential refresh. This is a significant refactoring that improves the API for distributed engines.

P1 Issues

1. Blocking Hash implementation - StorageOptionsAccessor::hash() uses blocking_read() on a tokio RwLock. This is called from the Hash trait which is sync. If this is called from an async context (which is likely given the codebase's async-first architecture), this could lead to deadlocks or panic. Consider either:

   • Using std::sync::RwLock for initial_options if it's rarely written to
   • Adding documentation warning about calling this from async contexts
   • Adding #[track_caller] or panic guards for async context detection

2. Test assertion on concurrent refresh - In test_accessor_concurrent_refresh, the assertion call_count >= 1 is very weak for a concurrency test. With 20 concurrent tasks hitting expired cache, you should validate that the provider isn't called 20 times (which would indicate the caching isn't working). Consider adding an upper bound like call_count <= 3 to catch regression where the cache stops working.

Minor Observations

• The consolidation of storage options and provider into StorageOptionsAccessor is a good API improvement.
• Good test coverage for the new accessor with various scenarios including expiration and concurrent access.
• Documentation in Python and Java APIs is comprehensive with clear examples.

The architecture change looks solid overall. The main concern is the blocking Hash impl which should be addressed before merge.