xiangfu0 commented on PR #19888: URL: https://github.com/apache/pinot/pull/19888#issuecomment-6203268246
**Automated review — head `2fba5c9` — LGTM (approve-equivalent; formal approve blocked because the review credential is the PR author account).** **Scope:** 24 files, +635/-54. Domains reviewed: backward-compat / mixed-version, correctness, concurrency, security/authorization, testing. **Assessment:** The query vs. ingestion Groovy separation is correct — the query path uses `forQuery()` / `parseGroovyScriptForQuery()` and is unaffected by the ingestion gate, while ingestion compilation fails closed by default. Server/minion startup registers the listener (immediate fail-closed snapshot), then applies the raw ZK cluster snapshot before any ingestion/query processing, then follows live Helix updates — fail-closed throughout, and `_ingestionGroovyDisabled`/`_disableGroovy`/`_clusterIngestionGroovyDisabled` are all `volatile`. Moving the access-control checks ahead of config validation in the `/tableConfigs` and `/schemas` endpoints is a genuine hardening (validation can compile Groovy, so it must not run before authorization). Good test coverage against the real compiler gate. **Minor findings (inline):** - `FunctionEvaluatorFactory`: the `MapFunctionEvaluator` comment claims Groovy `sort()` preserves a `SortedMap`'s comparator — it does not (Groovy forces ascending natural order). Equivalent for normal Avro `HashMap`/`LinkedHashMap` inputs, so no real ingestion impact; just reword the comment. - `TableConfigValidationUtils`: the new schema check matches the Groovy prefix case-insensitively (good), but the table-level checks in `TableConfigUtils` still use the case-sensitive `isGroovyExpression`. Defense-in-depth consistency note only — the compiler gate backstops it. **CI:** The two failing checks (`Java 11 Client Compatibility`, `Compat Regression vs release-1.5.0`) are transient Maven Central network errors (`Connection reset` fetching `checker-qual` / `google-cloud-core-bom`), not attributable to this change. `Binary Compatibility` and `Compat vs master` pass. Recommend a re-run. **Upgrade note (documented in the PR):** services now fail-close ingestion Groovy by default; set `controller.disable.ingestion.groovy=false` in cluster config before upgrading clusters that rely on ingestion Groovy. Also note this PR depends on #19235 and #19887 (merge first). -- 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]
