amogh-jahagirdar commented on code in PR #14867:
URL: https://github.com/apache/iceberg/pull/14867#discussion_r2893112408
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -595,9 +587,38 @@ private Supplier<BaseTable> createTableSupplier(
}
private RESTTable restTableForScanPlanning(
- TableOperations ops, TableIdentifier finalIdentifier, RESTClient
restClient) {
- // server supports remote planning endpoint and server / client wants to
do server side planning
- if (endpoints.contains(Endpoint.V1_SUBMIT_TABLE_SCAN_PLAN) &&
restScanPlanningEnabled) {
+ TableOperations ops,
+ TableIdentifier finalIdentifier,
+ RESTClient restClient,
+ Map<String, String> tableConf) {
+ // Get client-side and server-side scan planning modes
+ String clientModeConfig =
properties().get(RESTCatalogProperties.SCAN_PLANNING_MODE);
+ String serverModeConfig =
tableConf.get(RESTCatalogProperties.SCAN_PLANNING_MODE);
+
+ // Validate that client and server configs don't conflict
Review Comment:
cc @singhpk234 @RussellSpitzer Sorry for the late reply, I think I agree
with the latest spec verbage where we specify "MUST" and indicate that if a
client does not support that planning mode, they _should_ fail.
I still think there's really no need to fail if there's a mismatch between
what the client configured and what the server returned. The client
configuration isn't in the spec and so I think we have full choice on the
behavior in the case there's a mismatch. After some thought, I do think that
the right behavior in case of mismatch is to just use what the _server_ sent
back and log the warning you mentioned.
I take back what I said earlier about using what the client configured
because the protocol is to use what the server sent back if it did send back
anything and I think that is more consistent with how we think about general
config overrides as well?
I'm also good to move ahead if we really feel strongly about failing, I just
still maintain that it feels like an unnecessary behavior and there's
precedence from general config rules on how we should handle cases like this.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3468,6 +3468,9 @@ components:
## General Configurations
- `token`: Authorization bearer token to use for table requests if
OAuth2 security is enabled
+ - `scan-planning-mode`: Communicates to clients the supported scanning
mode. Clients should use this value to fail fast if the supported scanning mode
is not available on the client. Valid values:
Review Comment:
Minor: supported `planning` mode instead of `scanning`
--
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]
