Skip to content

Conversation

@jackye1995
Copy link
Contributor

@jackye1995 jackye1995 commented Dec 18, 2025

in distributed engines situation, we have the following problem with vended credentials: we pass in the namespace and table ID to get location and allow dynamic credentials refresh. Then the table is cached and used for serving multiple queries.

When executing in another worker (e.g. spark, lancedb enterprise, etc.), we have to basically fetch the credentials again because we don't know what is the current credentials to use, and the credentials could already been refreshed and is different from the initial input.

This PR adds an API dataset.current_storage_options() to get the latest storage options to be used, so that it can be served as the initial storage options to use in the worker node. This ensures we only make a single call to namespace_client.describe_table. Note that the engine should configure the credentials refresh lead time to be long enough that it is sufficient to use a single credential in the work in most cases.

Another problem is that when distributing to the worker, we already know the location of the table and the storage options to use, so we should just pass that in and use it. But today the API is an either-or, user either pass in uri or pass in namespace + table ID and it would reload uri and storage options. We added documentation and updated API so that if user pass in namespace + table_id, we do the automated workflow to get uri and storage options and set provider as usual, but also give caller the option to set each component manually to match various caching conditions.

@jackye1995 jackye1995 marked this pull request as draft December 18, 2025 23:09
@github-actions github-actions bot added enhancement New feature or request python java labels Dec 18, 2025
@jackye1995 jackye1995 changed the title feat: expose current_storage_options feat: make namespace related api more friendly for distributed engines Dec 18, 2025
@codecov
Copy link

codecov bot commented Dec 18, 2025

@jackye1995 jackye1995 force-pushed the expose-current-storage-options branch from bdff78f to 8dc1662 Compare December 19, 2025 08:07
@github-actions
Copy link
Contributor

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.

@github-actions
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request java python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant