gaborkaszab commented on code in PR #16247:
URL: https://github.com/apache/iceberg/pull/16247#discussion_r3217784106
##########
core/src/main/java/org/apache/iceberg/rest/RESTCatalogProperties.java:
##########
@@ -31,6 +31,7 @@ private RESTCatalogProperties() {}
public static final String SNAPSHOT_LOADING_MODE = "snapshot-loading-mode";
public static final SnapshotMode SNAPSHOT_LOADING_MODE_DEFAULT =
SnapshotMode.ALL;
public static final String SNAPSHOTS_QUERY_PARAMETER = "snapshots";
Review Comment:
`RESTSessionCatalog` uses `CatalogProperties.WAREHOUSE_LOCATION` for the
query param name. I'd recommend using the same param both at the client and the
server side. My preference would be to replace the one in
`RESTSessionCatalog.fetchConfig()` to use this one.
Additionally, when we populate this param on the client side for the
request, the code comments talks about warehouse location. I'm a bit confused.
Isn't this param a warehouse name, or is it a warehouse location?
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -540,6 +545,11 @@ public <T extends RESTResponse> T handleRequest(
return null;
}
+ /** Validates that the given warehouse exists. Subclasses may override. */
+ protected void validateWarehouse(String warehouse) {
+ // no-op: accept any warehouse by default
Review Comment:
The reason we leave the implementation to `RESTServerCatalogAdapter` is
because it has the catalog configs that we need to check for equality with the
warehouse query param, right?
This seems a bit odd to me. Can't we pass the catalog configs too to
`RESTCatalogAdapter`? Maybe pass `CatalogContext` via a new constructor?
Other option could be what the `applyCredentials` does. It doesn't go
through inheritance but we do the checks for response types and enhance with
credentials if needed. If we have to keep the warehouse checks on this level of
abstraction and not pass the catalog configs to `RESTCatalogAdapter`, we might
want to do the same: check for `ConfigResponse` and validate the param in
`vars`.
I'd personally go for the former option and pass `RESTCatalogAdapter`
whatever is needed to perform the check itself.
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -171,6 +173,9 @@ public <T extends RESTResponse> T handleRequest(
return castResponse(responseType, handleOAuthRequest(body));
case CONFIG:
+ if (vars.containsKey(RESTCatalogProperties.WAREHOUSE_QUERY_PARAMETER))
{
Review Comment:
nit: to keep this as simple as possible, can we simply do this and return
early if the warehouse is null?
```
validateWarehouse(vars.get(RESTCatalogProperties.WAREHOUSE_QUERY_PARAMETER));
```
--
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]
