DerGut commented on code in PR #2838:
URL: https://github.com/apache/iceberg-rust/pull/2838#discussion_r3652798926
##########
crates/catalog/rest/src/client.rs:
##########
@@ -58,189 +51,63 @@ impl Debug for HttpClient {
impl HttpClient {
/// Create a new http client.
- pub fn new(cfg: &RestCatalogConfig) -> Result<Self> {
- let extra_headers = cfg.extra_headers()?;
+ pub async fn new(cfg: &RestCatalogConfig) -> Result<Self> {
+ let auth_manager = cfg.resolve_auth_manager()?;
+ let session = auth_manager.init_session().await?;
Ok(HttpClient {
- client: cfg.client().unwrap_or_default(),
- token: Mutex::new(cfg.token()),
- token_endpoint: cfg.get_token_endpoint(),
- credential: cfg.credential(),
- extra_headers,
- extra_oauth_params: cfg.extra_oauth_params(),
+ client: cfg.client(),
+ extra_headers: cfg.extra_headers()?,
disable_header_redaction: cfg.disable_header_redaction(),
+ auth_manager,
+ session,
})
}
/// Update the http client with new configuration.
///
/// If cfg carries new value, we will use cfg instead.
/// Otherwise, we will keep the old value.
- pub fn update_with(self, cfg: &RestCatalogConfig) -> Result<Self> {
+ ///
+ /// The auth manager is kept; it derives a new session from the merged
+ /// properties (carrying over state such as a cached token).
+ pub async fn update_with(self, cfg: &RestCatalogConfig) -> Result<Self> {
+ let HttpClient {
+ // The same client comes back from `cfg.client()` below: the config
+ // clone shares the lazily-created default (or the user's client).
+ client: _,
+ extra_headers: current_headers,
+ disable_header_redaction: _,
+ auth_manager,
+ session: init_session,
+ } = self;
+ // Release the init-phase session before deriving the catalog session,
+ // so a manager whose init session guards a one-shot resource (released
+ // on drop) can build its catalog session without deadlocking.
+ drop(init_session);
Review Comment:
One thing I noticed while playing around with it a little more: we could
tighten the `AuthManager` trait to return a `Box` if we did the change now. In
my understanding, an init session is only meant to be used once and a public
API that locks this in might better express an init_session's intent.
```diff
-async fn init_session(&self) -> Result<Arc<dyn AuthSession>>
+async fn init_session(&self) -> Result<Box<dyn AuthSession>>
```
A catalog_session on the other hand is meant to be re-used (and shared by
concurrent requests). The current API (if made public) wouldn't convey that
difference.
--
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]
