adutra commented on code in PR #10753:
URL: https://github.com/apache/iceberg/pull/10753#discussion_r1828246578
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -378,7 +289,8 @@ public List<TableIdentifier> listTables(SessionContext
context, Namespace ns) {
paths.tables(ns),
queryParams,
ListTablesResponse.class,
- headers(context),
+ ImmutableMap.of(),
+ authManager.contextualSession(context, catalogAuth),
Review Comment:
I had initially something that resembles your suggestion. I discarded it for
two reasons:
1. `RESTClient` implementations are stateful (and closeable); "wrapping" a
REST client with the auth session may be error-prone: should implementors
return a new instance, or the same one? Should callers close the wrapped client
or not?
2. Wrapping makes the code slightly less testable: you don't "see" the
authentication data anymore by simply inspecting the calls that the catalog
made to the `RESTClient` API.
But I agree that the wrapping approach is less invasive, so let's give it
another try.
I think that to address concern #1 above, we can work around misbehaving
implementations with good documentation.
To address #2 above, we'd certainly need to revisit tests a little bit and
introduce a better way to make verifications, ideally capturing which requests
were made and what was the internal state of the REST client, by the time a
given request was executed.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
