gaborkaszab commented on code in PR #14060:
URL: https://github.com/apache/iceberg/pull/14060#discussion_r2368597708
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -426,7 +428,10 @@ public <T extends RESTResponse> T handleRequest(
case LOAD_TABLE:
{
LoadTableResponse response =
- CatalogHandlers.loadTable(catalog, tableIdentFromPathVars(vars));
+ CatalogHandlers.loadTable(
+ catalog,
+ tableIdentFromPathVars(vars),
+ snapshotModeFromQueryParams(httpRequest.queryParameters()));
Review Comment:
In my understanding the ETag describes a certain state of the table and the
REST request parameters aren't a factor when calculating an ETag. It doesn't
matter if `snapshots` is `ALL` or `REFS` because it's the same table, just for
the second case the snapshots are loaded later lazily if needed.
There won't be cache collisions because of this because if a table is
initially loaded with `REFS` then the reduced snapshot list is going to be
cached on RESTSessionCatalog-side, and when loading the full snapshot list
on-demand, it won't go through RESTSessionCatalog and won't update the cache.
This [art of the implementation is something I'm working on, so based on
what we store in the client-side cache we either get the reduced snapshot list
all the time from loadTable and then rest could be loaded on-demand, or we
could either update the cached Table object with the on-demand loading and then
further loadTable request would get the full snapshot list. But this won't
cause cache collisions in my opinion.
--
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]
