plusplusjiajia commented on code in PR #2838:
URL: https://github.com/apache/iceberg-rust/pull/2838#discussion_r3680771894
##########
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:
@DerGut Thanks for trying it on a branch — that shape looks like the natural
target when the manager moves into the catalog later. And done on the Box
suggestion: init_session now returns Box<dyn AuthSession>, catalog_session
keeps Arc, docs spell out the distinction.
--
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]
