DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r3973924777
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -416,10 +418,14 @@ pub(crate) fn oauth_params_from_props(props:
&HashMap<String, String>) -> HashMa
params
}
-#[derive(Debug)]
struct RestClient {
- /// Carries the session the auth manager derived from the merged
- /// configuration, so every request below is authenticated.
+ /// The manager that created `catalog_session`; retained so each request
can
+ /// derive authentication for its [`SessionContext`].
+ auth_manager: Arc<dyn AuthManager>,
+ /// The catalog-wide session passed to [`AuthManager::contextual_session`].
+ catalog_session: Arc<dyn AuthSession>,
Review Comment:
There's actually something more to it. The catalog session depends on the
`/v1/config` handshake and on the init session. Because the `RestClient` is
initialized lazily, it's not trivial to simply move the field to the
`RestSessionCatalog`.
I see three options:
1. keep them in the `OnceCell<RestClient>` and re-use the lazy intialization
2. introduce new `OnceCell<Arc<dyn AuthManager>>` and `OnceCell<Arc<dyn
AuthSession>>` and have their initialization depend on the rest client's
initialization
3. use a wrapper to encapsulate the rest client, auth manager and catalog
session into a single lazy initialization
My thoughts on them:
2. is spreading lazy initialization logic around the code where it's really
only one lazy chain that could be simplified
3. I could see this one work out but I haven't found a good name yet. I also
am concerned that we're building a deep tree of encapsulating types:
`RestCatalog` -> `RestSessionCatalog` -> `<SomeNewType>` -> `RestClient` ->
`HttpClient`. The distinction between the `RestClient` and `HttpClient` isn't
too well-defined yet (it's essentially an http client with rest-catalog
specific config). Maybe this could be an opportunity to give it a more specific
responsibility to save us from another layer -> that would get us to a hybrid
of 1. and 3.
--
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]
