Core: Extended header support for RESTClient implementations #12194

gaborkaszab · 2025-02-07T17:05:31Z

The freshness-aware table loading requires some additional support for HTTP headers:

Response headers for get and post requests
Input headers for get request

Extended the RESTClient and its implementations to fill in the gaps where these headers weren't supported.

gaborkaszab · 2025-02-10T12:39:54Z

Conflict with this refactor: #11992
Will take care of the rebase soon.

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java

1raghavmahajan

Wouldn't it be better to allow users to provide their own HTTPClient implementation here and leverage HTTP Caching in a transparent manner while also exposing cache control/size?
This simplifies the flow from Iceberg perspective as well. I think it's better to avoid explicit header handling when it could be done via a compliant HTTP client.

1raghavmahajan · 2025-03-05T13:35:00Z

To make things simpler from a user perspective we could also add a default CachedHttpClient to the builder in case someone wants something that just works out of the box.

gaborkaszab · 2025-03-05T17:10:52Z

Thanks for taking a look, @1raghavmahajan!

This is the proposal doc the freshness-aware table loading just in case: https://docs.google.com/document/d/1rnVSP_iv2I47giwfAe-Z3DYhKkKwWCVvCkC9rEvtaLA

In general the whole improvement could work with letting the HttpClient to take care of the caching out of the box itself as you proposed.
However, The idea is to do the caching on RESTSessionCatalog side so that we can cache Table objects instead of the HTTP messages. Additionally, if the caching was on RESTSessionCatalog level then the cached objects could be tied to the lifecycle of the tables, e.g. dropping a table could also evict the table object from the cache (via using weak-references) while the CachedHttpClient would only see HTTP messages.

So I'd continue with this PR to add extended header support for Iceberg's REST clients.

danielcweeks · 2025-03-20T20:48:57Z

core/src/main/java/org/apache/iceberg/rest/BaseHTTPClient.java

  }

+  @Override
+  public <T extends RESTResponse> T get(


If we're adding this, would it be possible to push the other get implementation to the interface?

Sorry for the late response.

Do you mean we could move that other get function from this class into the interface? We could have a "responseHeader-less" version in the interface that calls the version with the responseHeader param using a non-null h -> {} consumer, but I think that would be a behaviour change within the interface. Calling the "responseHeader-less" version would result in an UnsupportedOperationException from the default implementation of the other get function in the interface.

With the current approach I tried to follow how this PR from @nastra introduced the same for the post methods.

zratkai · 2025-03-31T12:30:09Z

core/src/main/java/org/apache/iceberg/rest/HTTPClient.java

   * additional resource allocation.
   */
-  private HTTPClient(HTTPClient parent, AuthSession authSession) {
+  HTTPClient(HTTPClient parent, AuthSession authSession) {


Please use the builder of this class instead of changing the visibility of the constructor.

In fact, I'll revert this part (the second commit) from this PR because that was a kind of experiment to see how we could test the new interface in HTTPClient. Agreed, with @nastra that this isn't the way we'd want to go.

gaborkaszab · 2025-04-02T10:54:01Z

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 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

github-actions · 2025-05-03T00:16:35Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

github-actions · 2025-05-11T00:18:41Z

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

gaborkaszab · 2025-05-12T10:56:31Z

Hi @danielcweeks , @nastra ,

I left this PR to expire since I plan to come up with a wider code change that covers more parts of the freshness aware loading. I just wanted to let you know that I recently had some shift in priorities and didn't have time to work in this, but in some weeks I'll again find capacity to continue the implementation. I have some open question wrt the design, I might reach out to you if you don't mind.
Thanks!

dramaticlly · 2025-07-30T21:42:24Z

@gaborkaszab @nastra @danielcweeks I am interested in this change to configure/expose the etag headers on GET request, would love to push this forward in a separate PR, please let me know if there's any WIP effort on this.

danielcweeks · 2025-07-30T22:04:28Z

@dramaticlly I don't think anyone is actively working this, so if you want to pick it up and move forward, I think that would be great.

gaborkaszab · 2025-07-31T07:36:23Z

@dramaticlly @danielcweeks I admit I got a bit sidetracked from this, but still this is on my plate, so I'd be a bit uncomfortable to give it away. Can we coordinate on intentions here before we move one?

Anyway, this was my last message on this PR:

I just wanted to let you know that I recently had some shift in priorities and didn't have time to work in this, but in some weeks I'll again find capacity to continue the implementation

dramaticlly · 2025-08-18T21:48:12Z

@gaborkaszab do you want to take a look at suggestion from @nastra? Happy to help in any way

…n ETags The freshness-aware table loading requires some additional support for HTTP headers: - Response headers for get and post requests - Input headers for get request Extended the RESTClient and its implementations to fill in the gaps where these headers weren't supported. With this patch, RESTCatalogAdapter populates the ETag HTTP response header.

gaborkaszab

Thanks for taking a look @nastra and @dramaticlly !
I've taken care of your comments except the one with ETags for stageCreate. I think there we should have some additional discussion.

gaborkaszab · 2025-08-19T09:36:12Z

core/src/main/java/org/apache/iceberg/rest/ETagProvider.java

+import org.apache.iceberg.rest.responses.LoadTableResponse;
+
+/** Interface for creating the content of the ETag HTTP headers */
+public interface ETagProvider {


removed the interface and kept the utility class with a static method

gaborkaszab · 2025-08-19T09:36:22Z

core/src/test/java/org/apache/iceberg/rest/DefaultETagProvider.java

+import org.apache.iceberg.relocated.com.google.common.hash.Hashing;
+import org.apache.iceberg.rest.responses.LoadTableResponse;
+
+public class DefaultETagProvider implements ETagProvider {


gaborkaszab · 2025-08-19T09:37:05Z

core/src/test/java/org/apache/iceberg/rest/DefaultETagProvider.java

+  private static final HashFunction MURMUR3 = Hashing.murmur3_32_fixed();
+
+  @Override
+  public String of(LoadTableResponse resp) {


thanks @nastra @dramaticlly ! changed the parameter to metadata location

gaborkaszab · 2025-08-19T09:37:14Z

core/src/test/java/org/apache/iceberg/rest/DefaultETagProvider.java

+  private static final HashFunction MURMUR3 = Hashing.murmur3_32_fixed();
+
+  @Override
+  public String of(LoadTableResponse resp) {


gaborkaszab · 2025-08-19T09:37:27Z

core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java

    return this;
  }

+  public RESTClient withETagProvider(ETagProvider eTagProv) {


gaborkaszab · 2025-08-19T09:51:57Z

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java

+
+    respHeaders.clear();
+
+    Table tbl = catalog.loadTable(TABLE);


gaborkaszab · 2025-08-19T09:52:16Z

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java

+        TableIdentifier.of(TABLE.namespace(), "other_table"),
+        ((BaseTable) tbl).operations().current().metadataFileLocation());
+
+    assertThat(respHeaders).containsKey(HttpHeaders.ETAG);


gaborkaszab · 2025-08-19T10:04:18Z

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java

+    assertThat(eTag).isEqualTo(respHeaders.get(HttpHeaders.ETAG));
+  }
+
+  private RESTCatalog setUpETagTest(Map<String, String> respHeaders) {


gaborkaszab · 2025-08-19T10:04:32Z

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java

+
+    RESTCatalog catalog = catalog(adapter);
+
+    if (requiresNamespaceCreate()) {


gaborkaszab · 2025-08-19T12:30:05Z

core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java

          request.validate();
          if (request.stageCreate()) {
            return castResponse(
                responseType, CatalogHandlers.stageTableCreate(catalog, namespace, request));


I wasn't sure here. Isn't the metadata location null for stage create? I created a simple test and it was null there, so I don't think we can add an ETag here. Do I miss something?

core/src/main/java/org/apache/iceberg/rest/ETagProvider.java

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java

nastra · 2025-08-20T14:28:01Z

core/src/main/java/org/apache/iceberg/rest/ETagProvider.java

+import org.apache.iceberg.relocated.com.google.common.hash.HashFunction;
+import org.apache.iceberg.relocated.com.google.common.hash.Hashing;
+
+class ETagProvider {


nit: it might make sense to have a small test class for this where the metadata location is null/well-defined and where we compare against a precalculated etag value

by recalculated tag value do you mean that a test where the expected output is hard-coded? Would that guard against someone changing the implementation of ETag creation? I added a test for that, let me know if this is what you mean

dramaticlly

LGTM, Thanks @gaborkaszab!

dramaticlly · 2025-08-27T21:08:47Z

@nastra @pvary do we need anything else before we can merge this change?

gaborkaszab · 2025-09-04T11:46:47Z

According to Slack, @nastra seems to be offline until mid Sept. Maybe @amogh-jahagirdar would you mind taking a look? I got some approvals previously already, I just addressed a nit on top to add some extra tests.

pvary · 2025-09-08T12:01:23Z

Merged to main.
Thanks @gaborkaszab for the PR and for all the reviewers!

github-actions bot added core OPENAPI labels Feb 7, 2025

nastra self-requested a review February 11, 2025 16:16

gaborkaszab force-pushed the main_rest_response_headers branch from c214964 to 0ee6b38 Compare February 13, 2025 12:21

nastra reviewed Feb 19, 2025

View reviewed changes

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java Show resolved Hide resolved

nastra changed the title ~~REST: Extended header support for RESTClient implementations~~ Core: Extended header support for RESTClient implementations Feb 19, 2025

gaborkaszab requested a review from nastra March 4, 2025 13:41

nastra requested a review from danielcweeks March 4, 2025 15:41

1raghavmahajan reviewed Mar 5, 2025

View reviewed changes

gaborkaszab force-pushed the main_rest_response_headers branch 2 times, most recently from 752eb00 to 7ad2f3f Compare March 7, 2025 12:53

danielcweeks reviewed Mar 20, 2025

View reviewed changes

zratkai reviewed Mar 31, 2025

View reviewed changes

gaborkaszab force-pushed the main_rest_response_headers branch from 7ad2f3f to df18911 Compare April 2, 2025 09:40

gaborkaszab force-pushed the main_rest_response_headers branch from df18911 to 744bb96 Compare April 2, 2025 11:15

github-actions bot added the stale label May 3, 2025

github-actions bot closed this May 11, 2025

dramaticlly mentioned this pull request Jul 31, 2025

REST: Expose response headers in GET requests #13712

Closed

gaborkaszab reopened this Jul 31, 2025

gaborkaszab force-pushed the main_rest_response_headers branch from 744bb96 to 4e7fe97 Compare July 31, 2025 08:54

gaborkaszab force-pushed the main_rest_response_headers branch from e834987 to 669b44e Compare August 19, 2025 13:20

gaborkaszab commented Aug 19, 2025

View reviewed changes

gaborkaszab requested a review from nastra August 19, 2025 14:29