laserninja opened a new issue, #12254: URL: https://github.com/apache/gravitino/issues/12254
### What would you like to be improved? The Iceberg REST server's scan plan cache is off by default: `gravitino.iceberg-rest.scan-plan-cache-impl` has no default value, so unless an operator sets it, every scan planning request plans from scratch. That was cheap to leave off while `POST .../plan` was the only scan planning endpoint. It is less cheap now that scan planning hands results out in batches ([#11284](https://github.com/apache/gravitino/issues/11284), [#12194](https://github.com/apache/gravitino/pull/12194)): a `plan-task` is resolved by replanning the snapshot it pins, so a plan spanning N batches costs N plans with the cache off and one with it on. | Plan size | Batch size (default 100) | Client requests | Plans computed, cache off | Plans computed, cache on | | --------- | ------------------------ | --------------- | ------------------------- | ------------------------ | | 80 | 100 | 1 | 1 | 1 | | 1,000 | 100 | 10 | 10 | 1 | | 100,000 | 100 | 1,000 | 1,000 | 1 | Replanning is not incorrect - the snapshot is pinned, so every replan of the same plan task yields the same tasks - but it reads the same manifests repeatedly for a result the server already computed. The same applies to repeated identical scans from BI tools and dashboard refreshes, which is the case the cache was built for in the first place. ### How should we improve? Decide whether `LocalScanPlanCache` should be the default `scan-plan-cache-impl`, in the way `LocalTableMetadataCache` is already the default `table-metadata-cache-impl`. Points worth settling in this issue: 1. **Default on or off.** Enabling it makes batched scan planning cheap out of the box. It also means a server that plans scans holds cached plan responses in memory without an operator asking for it. 2. **Memory footprint.** The existing defaults are `scan-plan-cache-capacity=200` entries and `scan-plan-cache-expire-minutes=60`. An entry holds a whole plan response, so 200 entries of a large plan is a meaningful amount of heap. If the cache becomes the default, these defaults probably deserve a second look, or a size-aware bound rather than an entry count. 3. **Correctness is not at stake.** `ScanPlanCacheKey` includes the resolved snapshot id and every scan parameter, so a cached plan is only served to an identical scan of the same snapshot; enabling the cache cannot return stale tasks. 4. **Multi-replica deployments.** A node-local cache only helps when the follow-up request lands on the same replica. A shared implementation would help every replica, and is tracked separately as a possible follow-up. Context: this was raised while reviewing the scan planning design document ([#12241](https://github.com/apache/gravitino/pull/12241)), where the decision was to keep caching out of that design and discuss the default here instead. @lasdf1234 stated a preference for enabling it. -- 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]
