gaborkaszab commented on PR #12194: URL: https://github.com/apache/iceberg/pull/12194#issuecomment-2772189580
Just sharing some updates for the record: We discussed with @eduard previously that I should have a test that verifies that the headers are populated with this implementation. I investigated different ways to achieve that, but apparently this part of the code is not that easy to mock. The reason in a nutshell is that even if I provide a custom `HTTPClient` to the `RESTCatalog` in the tests, when the catalog is initialised the `withAuthSession` call returns a new `HTTPClient` instance and hence we loose the override for functions. The only way I managed to make this part of the code testable required me changing the visibility of the `HTTPClient` constructor to public, but this is not something we'd want to do. So I [reverted that part of the code](https://github.com/apache/iceberg/compare/7ad2f3fbee047c0999c49257b7a4c68929688ac5..df189119a0ba0cb359b5d1ad273b110b1193015c) from this PR. I had limited capacity to follow-up on this recently, but this changes soon hopefully. I continued with the implementation of the freshness-aware loading, but I don't think there is an easy way anyway to test this part of the code as a separate building block. I wonder if we can merge this one as it is (with the additional test reverted) to have granularity with the implementation. @nastra @danielcweeks -- 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
