[raunaqmorarka]

RESTTableOperations.refresh() always issued an unconditional GET and parsed the full metadata body, even though the same catalog already does freshness-aware loading for loadTable through RESTTableCache. Servers that honour If-None-Match (for example Polaris) answer with 304 Not Modified, so a refresh of an unchanged table only needs a header round trip.

RESTTableOperations now takes the ETag of the load response through its constructor, updates it from every successful refresh and commit response, sends it as If-None-Match on refresh(), and returns current() on a 304 without parsing a body. When the server sends no ETag, refresh behaves as before. The RESTTableCache entry is left untouched, since the ETag there pairs with the metadata captured by the cached supplier.

RESTSessionCatalog.newTableOps gets a new overload with an eTag parameter. The existing one is deprecated and delegates with null.

Note: commit() now uses the RESTClient.post overload that returns response headers. BaseHTTPClient supports it. A custom RESTClient relying only on the interface default would need to implement it.

Tests in TestFreshnessAwareLoading cover a 304 on refresh after loadTable, a changed table refreshing fully and adopting the new ETag, a commit ETag being reused, and servers that send no ETag. The test adapter now treats a missing snapshots param as all when computing the ETag, as the spec says.

[gaborkaszab]

Isn't this solving the same as #17832?

[raunaqmorarka]

Isn't this solving the same as #17832?

Different problem. #17832 restores the lazy snapshots supplier and the snapshots query parameter on refresh in refs mode. This PR makes refresh conditional with If-None-Match so an unchanged table gets a 304 and no metadata body is parsed. Both change refresh() and updateCurrentMetadata, so one will need a rebase after the other merges, but they are independent. They also fit together: with #17832 the refresh request carries the same query parameters as the load, so the ETag from loadTable matches on the first refresh, and a 304 leaves current() and its snapshots supplier untouched.

[gaborkaszab]

Hi @raunaqmorarka ,

I'm trying to refresh my memories since I was involved with this. Couple of questions:

1. Isn't the ETag redundant now: There is one kept in the table cache and one in ops. Also, they can get out of sync, right?
2. In terms of the table cache, this is an in-place operation, right? Executing ops.refresh() also refreshes the TableMetadata in the table cache through ops, right?
3. I recall one complication might be that credentials and configs are also returned by the get request not just table metadata and ETag. Since these are incorporated into FileIO and RESTClient, they are not refreshed, right? Since the age of the table in the cache is not affected here, probably we are fine with this, just want to double check.

cc @nastra @danielcweeks Might be a useful addition. I always dragged this as a potential future addition to freshness-aware loading. Would you mind also taking a look?

[raunaqmorarka]

1. The two ETags validate different metadata. The cache ETag belongs to the metadata captured in the cached supplier, and the ops ETag belongs to ops.current(). They diverge after a refresh or commit, but each stays paired with its own metadata. Writing the refreshed ETag back into the cache would be wrong, since the cached supplier still returns the load-time metadata and a 304 on loadTable would then hand out stale metadata. The cost of divergence is one extra full load on the next loadTable, which then replaces the cache entry.

2. Correct, it is not in place. The cached supplier creates a new RESTTableOperations on every get(), so ops.refresh() only updates that instance. The cache entry keeps the load-time metadata and ETag.

3. refresh() never consumed config or credentials from the response before this change either. Only loadTable feeds them into the table FileIO. A 304 has no body, so nothing changes on that path, and the cache entry with its FileIO is untouched.

[gaborkaszab]

Thanks!
I did a pass on the non-test part. Will cover tests later.

[gaborkaszab]

I went another round, this time also checked tests

[gaborkaszab]

What are the params used by the catalog adapter that aren't the same used by ops?
Is this deviation of params general and expected between the catalog client and ops? Because if we expect this to be the case, then there isn't much point of constructing ops passing an ETag from RESTSessionCatalog, because anyway the first refresh() is expected to be a full load.

[raunaqmorarka]

Good question. The difference was the snapshots param. loadTable sends snapshots=all, while refresh() omits it. The test adapter hashed those params into the ETag, so the two never matched. Both requests return the same content, so I updated the adapter to treat a missing snapshots as all. The first refresh after loadTable now gets a 304. Only snapshot-loading-mode=refs still does a full first refresh, since its content differs.

[gaborkaszab]

Whenever we decide to send a new query param, this test will fail on the header verification because of how we calculate the ETag.
Also, the comment above is pretty misleading together with this new code because the comment says not sending snapshots param, but we anyway use it right below the comment. (I know why it's needed, but still might be misleading for the reader)

Do you think you can figure out a more flexible verification? For instance ETag content is not that relevant for this test, we still make changes so that we can make the rigid way of our message verification pass. Would be nice to have a way saying "I'm not interested verifying the outgoing headers" or "I'm just verifying the name of the outgoing headers but not their values"

[raunaqmorarka]

Agreed, that was brittle. I restored this test to how it is on main. I also added a containsHeaders overload to RequestMatcher that checks the query params and only the headers the test names. The test no longer depends on the ETag.

[gaborkaszab]

I'm not entirely sure what coverage we add with this test. The step with the commit() probably is not needed. I guess to point is to see what happens when the server sends no ETag. Then the plan for this test might be:

1. Construct server not to send ETag
2. Create and load table through catalog
3. do a refresh()
4. Verify refresh() didn't send and didn't receive an ETag
5. Verify that the table metadata is as we expect

[raunaqmorarka]

Thanks for the outline, I followed it. The test is now refreshWithoutETag, with no commit step. It checks that refresh sends no If-None-Match and keeps the loaded metadata.

[raunaqmorarka]

@gaborkaszab @nastra @pvary @amogh-jahagirdar PTAL