adutra commented on code in PR #18756:
URL: https://github.com/apache/iceberg/pull/18756#discussion_r4453407719
##########
core/src/main/java/org/apache/iceberg/rest/RESTClient.java:
##########
@@ -226,4 +226,14 @@ <T extends RESTResponse> T postForm(
default RESTClient withAuthSession(AuthSession session) {
return this;
}
+
+ /**
+ * Unwraps this client to the given type, if possible. This allows callers
to access the
+ * underlying HTTP library used by this client.
+ *
+ * @return the unwrapped client, or null if this client cannot be unwrapped
to the given type
+ */
+ default <T> T unwrap(Class<T> type) {
+ return type.isInstance(this) ? type.cast(this) : null;
+ }
Review Comment:
Very good remarks!
1. On the shred client: I'm following the existing `AuthManager` contract:
`initSession` and `catalogSession` hand the catalog’s `RESTClient` to the
manager so auth traffic reuses its configuration, and the legacy manager has
always sent token requests through it, including to external IdPs. What is
shared is the Apache-level configuration (pool, proxy, TLS/TLSConfigurer,
timeouts, user agent, retry strategy); the catalog’s `header.*` base headers
are not sent to the authorization server, since the adapter sits below
`HTTPClient`. I agree a deployment could need a different proxy or truststore
for the IdP; I’d rather add that as an additive override
(`rest.auth.oauth2.client.*` on top of `rest.client.*`, only building a second
client when set) in a follow-up than require everyone to configure two clients
up front.
2. On the `unwrap()` method: The manager needs a raw request/response
exchange that `RESTClient` doesn’t expose: the existing methods deserialize
into `RESTResponse` types (which Nimbus doesn't recognize), run errors through
`ErrorHandler`s, and `postForm()` is single-valued so it can’t carry a repeated
resource parameter (RFC 8707). Building our own Apache client would duplicate
`HTTPClient.Builder`’s proxy/TLS/timeout handling; adding a raw-exchange method
to `RESTClient` is a larger API change that every implementation would have to
support. Therefore, `unwrap()` seemed the smallest step: by default it returns
null, so a `RESTClient` not backed by Apache `HttpClient` fails with a clear
message.
--
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]
