DerGut commented on code in PR #2838:
URL: https://github.com/apache/iceberg-rust/pull/2838#discussion_r3652702139
##########
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:
I think the [same
argument](https://github.com/apache/iceberg-rust/pull/2838/changes#r3650396684)
holds here - we can still do it later.
Also thanks for writing the test in a behavioral way that allows to test
other approaches. I was able to construct the `init_session` once in the
`get_or_try_init` RestContext construction (on a [test
branch](https://github.com/apache/iceberg-rust/compare/main...DerGut:iceberg-rust:refactor/authmanager-in-catalog?expand=1)
based on yours) and directly passed it to the `RestCatalog::load_config` call.
Its lifetime is then constrained to only that constructor only and still passes
the test.
I then kept a reference to the `catalog_session` on the RestContext and put
a helper to always use that session on other `query_catalog` calls.
--
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]
