DerGut commented on code in PR #2838:
URL: https://github.com/apache/iceberg-rust/pull/2838#discussion_r3647773192
##########
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 wonder whether we can instantiate the `init_session` in the scope of its
use (the first `/v1/config` request) so that we don't have to deal with
explicit drops.
This could be
[another](https://github.com/apache/iceberg-rust/pull/2838/changes#r3634014541)
signal that the AuthManager should rather live in the catalog because the
`HttpClient` is not aware of which request is being made, and so it can't tell
which session is the appropriate one to use (or to build).
In that sense, it's implicitly temporally coupled to what the session field
has been set to, and has to assume that the first request being made is a
`/v1/config` request.
--
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]
