laserninja opened a new pull request, #12320: URL: https://github.com/apache/gravitino/pull/12320
### What changes were proposed in this pull request? Make `LocalScanPlanCache` the default `gravitino.iceberg-rest.scan-plan-cache-impl`, in the same way `LocalTableMetadataCache` is already the default `table-metadata-cache-impl`. - `IcebergConfig.SCAN_PLAN_CACHE_IMPL` now defaults to `org.apache.gravitino.iceberg.service.cache.LocalScanPlanCache`. The class lives in the Iceberg REST server module, which `iceberg-common` does not depend on, so it is referenced by name and pinned by a test. - Setting the property to an empty string (`""`) remains the way to turn caching off; `CatalogWrapperForREST#loadScanPlanCache` already falls back to `ScanPlanCache.DUMMY` for a blank value, so no server-side change was needed. - Capacity (200) and expiry (60 minutes) defaults are unchanged. - Docs updated with the new default, how to disable it, the per-catalog heap footprint, and a note that `rest`-backed catalogs delegate planning upstream and so do not use the cache. ### Why are the changes needed? With the cache off by default, every scan planning request plans from scratch, including the repeated identical scans from BI tools and dashboard refreshes the cache was built for. It also becomes the dominant cost once scan planning hands results out in batches (#11284, #12194), where redeeming a `plan-task` replans the pinned snapshot: a plan spanning N batches costs N plans with the cache off and one with it on. Correctness is unaffected: `ScanPlanCacheKey` includes the resolved snapshot id plus every scan parameter Gravitino acts on, so a cached plan is only served to an identical scan of the same snapshot. The wider cache design questions raised in the issue (size-aware bounding instead of an entry count, separate tiers, multi-replica/distributed caching) are deliberately left out of this PR and remain open in #12254. Fix: #12254 ### Does this PR introduce _any_ user-facing change? Yes. `gravitino.iceberg-rest.scan-plan-cache-impl` changes from no default to `org.apache.gravitino.iceberg.service.cache.LocalScanPlanCache`, so scan plan caching is on out of the box. A server that plans scans now holds up to `scan-plan-cache-capacity` (200) plan responses per catalog on the heap for `scan-plan-cache-expire-minutes` (60) after last access. Operators who do not want that can set `scan-plan-cache-impl` to an empty string. No property keys were added or removed. ### How was this patch tested? New unit tests in `TestCatalogWrapperForREST`: - `testScanPlanCacheDefaultImpl` - the config default resolves to `LocalScanPlanCache` (pins the class name used in `IcebergConfig`). - `testPlanTableScanCachedByDefault` - with no cache configuration, two identical scans of the same snapshot return the same response instance, i.e. the second is served from the cache. - `testPlanTableScanCacheDisabledByEmptyImpl` - with `scan-plan-cache-impl=""`, the same two scans replan. Existing suites pass: `./gradlew :iceberg:iceberg-rest-server:test :iceberg:iceberg-common:test :catalogs:catalog-lakehouse-iceberg:test -PskipITs`, plus `spotlessCheck`. -- 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]
