bharos commented on code in PR #11634:
URL: https://github.com/apache/gravitino/pull/11634#discussion_r3908812768
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/FederatedCatalogWrapper.java:
##########
@@ -188,12 +198,109 @@ public PlanTableScanResponse planTableScan(
catalogCredentialManager.catalogName(), tableIdentifier, response);
}
+ /**
+ * Reports whether the remote catalog advertises the scan-plan endpoint,
since {@link
+ * #planTableScan} delegates planning to it rather than planning locally.
+ *
+ * <p>The answer comes from the remote catalog's own {@code /v1/config}
response and is cached for
+ * the lifetime of this wrapper, so the remote is queried at most once
rather than on every local
+ * {@code /v1/config} call.
+ *
+ * <p>A remote that omits {@code endpoints} is treated as not supporting
scan planning. That
+ * matches the Iceberg client, which falls back to a default endpoint set
that predates scan
+ * planning when the field is absent.
+ *
+ * <p>If the remote cannot be reached the result is not cached and the
endpoint is not advertised,
+ * so a later call can still resolve it once the remote recovers. Not
advertising is the safe
+ * direction here: the endpoint would fail anyway while the remote is
unreachable.
+ *
+ * @return {@code true} if the remote catalog advertises {@code
V1_SUBMIT_TABLE_SCAN_PLAN}.
+ */
+ @Override
+ public boolean supportsScanPlanOperations() {
+ Boolean cached = remoteSupportsScanPlan;
+ if (cached != null) {
+ return cached;
+ }
+
+ try {
+ boolean supported =
+
fetchRemoteConfig().endpoints().contains(Endpoint.V1_SUBMIT_TABLE_SCAN_PLAN);
+ remoteSupportsScanPlan = supported;
+ return supported;
+ } catch (Exception e) {
+ LOG.warn(
+ "Failed to read the endpoints advertised by the remote catalog of
{}; not advertising the"
+ + " scan plan endpoint",
+ catalogCredentialManager.catalogName(),
+ e);
+ return false;
+ }
+ }
+
+ /**
+ * Fetches the remote catalog's {@code /v1/config} response.
+ *
+ * <p>The {@code warehouse} query parameter is forwarded when configured, so
a remote serving
+ * several warehouses returns the endpoint set for the one this catalog
federates.
+ *
+ * @return the remote catalog's config response.
+ */
+ @VisibleForTesting
+ ConfigResponse fetchRemoteConfig() {
Review Comment:
Two small things on this method, neither blocking:
1. Can this be `private`? `fetchRemoteConfig` is only called from
`supportsScanPlanOperations` — the new tests inject through the `getCatalog()`
override rather than calling this directly, so `@VisibleForTesting` isn't quite
accurate. (The import can go too if nothing else in the file uses it.)
2. Worth one line of javadoc on *why* it refetches.
`RESTCatalog.initialize()` already does a `GET /v1/config` against this same
remote, but `RESTSessionCatalog` keeps `endpoints` in a private field with no
accessor, so it can't be read back — the refetch is the only option. Without
that noted, this reads as a redundant round trip and someone will eventually
try to remove it. Something like:
```java
* <p>{@code RESTCatalog} already fetched this at init but keeps the
endpoint set private, so it
* has to be re-fetched here.
```
--
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]
