Repository navigation
Conversation
a9b963c to
e1a4d05
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19887 +/- ##
============================================
+ Coverage 69.42% 69.45% +0.02%
Complexity 1491 1491
============================================
Files 3545 3545
Lines 233979 233998 +19
Branches 37442 37446 +4
============================================
+ Hits 162448 162514 +66
+ Misses 58984 58939 -45
+ Partials 12547 12545 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
xiangfu0
left a comment
There was a problem hiding this comment.
Automated code review at e1a4d05 (CI green, 14/14). Verdict: LGTM / approve-equivalent — only minor, non-blocking notes below. GitHub blocks a formal APPROVE here because this review runs under the PR author's token, so this is posted as a comment.
Well-scoped hardening: the ingestion gate fails before compilation; query construction and validation correctly bypass it via forQuery/parseGroovyScriptForQuery while retaining the static-analysis controls; the legacy Avro __KEYS/__VALUES path moves off implicit Groovy without changing results.
I verified the map-extraction behavior against Groovy 3.0.25 directly: map.sort() preserves an existing SortedMap comparator and otherwise sorts by natural key order, which is exactly what MapFunctionEvaluator reproduces (reverse-ordered SortedMap, LinkedHashMap, HashMap, and empty map all match). No wire/serialization/SPI changes, so no mixed-version compatibility concern. Correctness, compatibility, and concurrency all look good.
| Map<?, ?> map = (Map<?, ?>) value; | ||
| // Groovy's sort() preserves the comparator of an existing SortedMap. | ||
| Map<?, ?> sortedMap = map instanceof SortedMap<?, ?> ? map : new TreeMap<>(map); | ||
| return new ArrayList<>(_extractKeys ? sortedMap.keySet() : sortedMap.values()); |
There was a problem hiding this comment.
Nice call reusing the map's own comparator here. I confirmed against Groovy 3.0.25 that map.sort() keeps an existing SortedMap comparator and natural-orders everything else, so this TreeMap fallback matches the old Groovy({...sort()...}) output exactly, including the reverse-ordered case. Behavior-preserving.
| private static final String SCRIPT_GROUP_NAME = "script"; | ||
| private static final String ARGUMENTS_SEPARATOR = ","; | ||
| private static GroovyStaticAnalyzerConfig _groovyStaticAnalyzerConfig; | ||
| private static volatile boolean _ingestionGroovyDisabled; |
There was a problem hiding this comment.
This is a JVM-wide latch toggled by a static setter. Fine for the service-config use, but the tests flip it and reset it in finally, so if any of these classes ever run in parallel in the same JVM they could observe each other's state. Just worth keeping in mind if test parallelism gets enabled later.
Expose GroovyFunctionEvaluator.setIngestionGroovyDisabled(boolean) for service runtime policy. Calling it with true rejects new ingestion evaluator construction and parseGroovyScript calls before compilation; calling it with false permits later ingestion compilation. Existing evaluators continue to execute. Ingestion Groovy stays enabled until a service configures the gate. Use forQuery and parseGroovyScriptForQuery at query execution and broker validation entry points so ingestion policy does not override query admission or Groovy static-analysis controls. Extract implicit Avro map keys and values directly in Java so disabling ingestion Groovy preserves legacy map ingestion, including SortedMap comparators. Validation: 66 focused evaluator, query-transform, expression-transform, and broker query-validation tests; affected-module formatting, checkstyle, and license checks.
e1a4d05 to
3a90f69
Compare
PR flow
Gate disables new ingestion Groovy compilation while preserving query Groovy and legacy map extraction.
AI-generated · Green: added · Yellow: modified · Red: removed · Gray: existing
Diff evidence
Add a shared gate that stops new ingestion Groovy compilation at evaluator construction and validation-script parsing. This protects ingestion of previously persisted configurations without passing policy parameters throughout ingestion APIs.
Query validation and both query evaluator construction paths explicitly retain query-time Groovy support under the existing query admission and static-analysis controls. Already compiled ingestion evaluators continue to execute.
Legacy Avro
__KEYS/__VALUESextraction uses Java instead of implicit Groovy, preserving key ordering, SortedMap comparators, and null handling when ingestion compilation is disabled.Usage
The service configuration layer controls the gate:
The shared evaluator retains its historical enabled default until a service configures it. #19888 applies the sole cluster setting,
controller.disable.ingestion.groovy, to servers and minions and disables ingestion compilation there by default. Controller admission is covered by #19235. No second Groovy enablement setting is introduced.Validation