[#11584] feat(iceberg-rest): Gate config endpoints by backend capabilities#11634
[#11584] feat(iceberg-rest): Gate config endpoints by backend capabilities#11634laserninja wants to merge 2 commits into
Conversation
laserninja
force-pushed
the
fix/11584-config-endpoint-capabilities
branch
from
364686c to
de16fa4
Compare
Code Coverage Report
Files
|
| IcebergCatalogWrapperManager icebergCatalogWrapperManager = | ||
| new IcebergCatalogWrapperManagerForTest( | ||
| catalogConf, configProvider, false, configProvider.getMetalakeName()); | ||
| IcebergCatalogWrapperManager icebergCatalogWrapperManager; |
There was a problem hiding this comment.
Why do we use reflection here?
There was a problem hiding this comment.
Reflection was used here to keep the overload generic, it accepts any Class<? extends IcebergCatalogWrapperManager> so tests can plug in different wrapper manager subclasses (e.g., NoScanPlanWrapperManager) without adding a new overload for each one.
That said, I can replace the reflection with a Supplier or a simple factory lambda instead, that's cleaner and avoids the unchecked reflection. Would you prefer that approach, or would you rather I just pass in an already-constructed instance?
There was a problem hiding this comment.
We should avoid reflection asap. Because reflection lacks necessary check in the compile stage. We can only find the issues in the runtime stage.
There was a problem hiding this comment.
Makes sense, agreed. I'll replace the reflection with a factory lambda (Function<..., IcebergCatalogWrapperManager>) so the constructor call is checked at compile time. Will push the update shortly.
Derive the advertised endpoint set from the backing catalog's capabilities rather than a static list. Gate V1_SUBMIT_TABLE_SCAN_PLAN behind CatalogWrapperForREST.supportsScanPlanOperations(), which returns true for non-REST backends (Hive, JDBC, Memory, Custom) and false for REST backends where the upstream catalog owns scan planning. Fixes apache#11584
laserninja
force-pushed
the
fix/11584-config-endpoint-capabilities
branch
from
de16fa4 to
ee403d9
Compare
2 participants
What changes were proposed in this pull request?
Derive the advertised endpoint set in
/v1/configfrom the backing catalog's capabilities rather than a hardcoded static list. Specifically, gate theV1_SUBMIT_TABLE_SCAN_PLANendpoint behindCatalogWrapperForREST.supportsScanPlanOperations(), which returnstruefor non-REST backends (Hive, JDBC, Memory, Custom) where Gravitino performs scan planning locally, andfalsefor REST backends where the upstream catalog owns scan planning.Why are the changes needed?
IcebergConfigOperationsadvertises a hardcodedDEFAULT_ENDPOINTSlist (includingV1_SUBMIT_TABLE_SCAN_PLAN) for every catalog, regardless of whether the configured backend actually supports those operations. A client that trusts the advertisedendpointsmay call an operation the backend cannot serve, producing confusing runtime errors.Fix: #11584
Does this PR introduce any user-facing change?
REST backend catalogs will no longer advertise the scan plan endpoint in the
/v1/configresponse. Clients that rely on the advertised endpoints will now correctly see only the operations their catalog backend supports.How was this patch tested?
TestIcebergConfigEndpointGatingwith tests verifying scan plan endpoint is omitted when not supported, and core endpoints are always present.testConfigEndpointsContainScanPlanForNonRESTBackendtoTestIcebergConfigverifying scan plan IS advertised for non-REST backends.