fix: stop tracker preheat from fully materializing OptionSet.options (master port of #24813) - #24815
Conversation
Adds per-option-set valid-code and resolved-set tracking so validators and the ASSIGN-action pipeline can check option codes without materializing the full OptionSet.options collection.
Queries optionvalue directly for the (option set, code) pairs actually referenced by the import payload, in chunks, instead of relying on OptionSetMapper to hydrate entire option collections. Cost is bounded by distinct codes referenced, not option set size. Includes the NOSONAR suppression for the two-phase collect/query loop (false positive: confirmed is populated by the query immediately before it's read).
Switches validateOptionSet() to check TrackerPreheat's valid-code cache instead of scanning OptionSet.getOptions(), with a diagnostic log.warn when an option set never got resolved during preheat. Updates all three call sites (tracked entity, enrollment, and event attribute/data value validators) and their tests accordingly.
OptionSetMapper no longer maps the options collection, which was forcing a full lazy-collection load (and JSONB deserialization of every option) for every OptionSet touched during tracker preheat. Callers now go through OptionValueSupplier/TrackerPreheat instead.
ASSIGN actions can introduce data element/attribute values that were never part of the original import payload, so OptionValueSupplier's single preheat pass can't have seen their option codes. Runs the assigned values back through OptionValueSupplier as a synthetic TrackerObjects before validation, so option-set validation still sees them as preheated. Adds getter access to the executors' resolved data element/attribute UID and value, and asserts no errors in the previously-unchecked option-value ASSIGN warning test.
…orTest SonarQube java:S5786 -- JUnit5 lifecycle methods don't need public visibility. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No regression on the existing
|
| Request | baseline p95 | candidate p95 | threshold |
|---|---|---|---|
| MNCH import | 89ms | 89ms | 140ms |
| Child Programme import | 64ms | 63ms | 115ms |
| ANC import | 42ms | 39ms | 71ms |
Flat to slightly faster on all three imports. The export/browse requests (ANC events, Child Programme TE search, etc.) show some percentage swings, but those are single-digit-ms deltas on 5-47 samples at SMOKE's single-user concurrency -- noise at that sample size, and none approach their own thresholds.
Full comparison table:
gstat compare, all requests
| Requests | baseline p95 | candidate p95 | Diff (ms) | Change |
|---|---|---|---|---|
| Get ANC events / Get one event / Get first event | 16 | 16 | -0 | -0.3% |
| Get ANC events / Get one event / Get relationships for first event | 5 | 5 | +0 | +0.0% |
| Get ANC events / Go to first page | 16 | 17 | +1 | +6.2% |
| Get ANC events / Go to second page | 18 | 18 | +0 | +0.0% |
| Get ANC events / Search not assigned | 16 | 17 | +1 | +5.9% |
| Get ANC events / Search by date range | 25 | 28 | +3 | +12.2% |
| Get Child Programme TEs / Go to single enrollment / Get one event / Get first event from enrollment | 26 | 25 | -1 | -3.8% |
| Get Child Programme TEs / Go to single enrollment / Get one event / Get relationships for first event | 5 | 5 | +0 | +0.0% |
| Get Child Programme TEs / Go to single enrollment / Get first tracked entity | 19 | 27 | +8 | +42.1% |
| Get Child Programme TEs / Go to single enrollment / Get first enrollment | 7 | 7 | +0 | +0.0% |
| Get Child Programme TEs / Go to single enrollment / Get relationships for first tracked entity | 5 | 6 | +1 | +20.0% |
| Get Child Programme TEs / Not found TE by name with like operator | 8 | 8 | +0 | +0.0% |
| Get Child Programme TEs / Not found TE by name with eq operator | 6 | 6 | +0 | +0.0% |
| Get Child Programme TEs / Search TE by name with like operator | 29 | 34 | +5 | +17.4% |
| Get Child Programme TEs / Search TE by name with eq operator | 23 | 28 | +5 | +22.0% |
| Get Child Programme TEs / Search Birth events | 33 | 30 | -3 | -9.1% |
| Get Child Programme TEs / Get TEs from events | 9 | 10 | +1 | +11.1% |
| Get Child Programme TEs / Get first page of TEs | 35 | 36 | +1 | +3.0% |
| Get Child Programme TEs / Get TEs with enrollment status | 44 | 47 | +3 | +6.8% |
| Login | 102 | 118 | +16 | +15.9% |
| MNCH import | 89 | 89 | -0 | -0.4% |
| Child Programme import | 64 | 63 | -1 | -1.2% |
| ANC import | 42 | 39 | -3 | -7.9% |
Bottom line: this PR produces no regression on the existing, unmodified TrackerTest -- all built-in assertions pass, import p95s flat-to-improved. (Companion perf coverage for the specific option-set-heavy scenario this fix targets is in #24814 / SingleEventTest, which shows the larger effect under a big option set and concurrent load.)
Relationship to the L2 cache "lock storm" work (#24803, #24810) — complementary, not a substituteThere's a separate, much more rigorous investigation into a Hibernate L2 cache "lock storm" Same
Both fixes independently eliminate master's hard-timeout failure mode — consistent with Practical takeaway: these are two separate, complementary fixes for the same option-heavy |
3 participants
Master port of #24813 (still open against
2.41, developed there first because aproduction-scale database was available to gather evidence). Same fix, same design — see
that PR for the original rationale. This PR adapts the
change for master's structural divergence since 2.41:
dhis-service-trackermodule →dhis-tracker(package unchanged)Eventbecame an interface (TrackerEvent/SingleEvent); preheat/rulecode here uses
TrackerEventUID-typed rather thanStringTrackerIdSchemeParammoved fromorg.hisp.dhis.tracker.importstoorg.hisp.dhis.trackerTrackerEvent/TrackedEntity/Enrollmentbuilders now require anon-null UID identifier field — this caught a real bug during porting
(see Testing below)
Fix
Tracker import preheat maps every referenced
OptionSetthroughOptionSetMapper. The generated MapStruct code called.size()on thelazy Hibernate collection backing
OptionSet.options, forcing a fullmaterialization of every
Optionrow for that set — including JSONBattributevaluesdeserialization per row — regardless of how many optioncodes the import payload actually references.
TrackerPreheatgainsaddValidOptionCode/isValidOptionCodeandaddResolvedOptionSet/isOptionSetResolved, storage for confirmed-valid(option set, code)pairs.OptionValueSupplierpreheat supplier walks the payload'sattribute/data values, collects only the option codes actually
referenced (splitting multi-text values), and resolves them with one
batched, chunked JDBC query against
optionvalue— cost bounded bypayload width, not option set size.
ValidationUtils.validateOptionSetreads from that preheated datainstead of
OptionSet.getOptions().OptionSetMapperstops mappingoptionsentirely — the expensivecollection load never happens during preheat.
ASSIGNactions can introduce option-set values that werenot in the original payload. Since those values are the rule engine's
already-evaluated output, they're resolved the same way, right after the
rule engine runs and before any validation reads them.
Performance validation (master, this PR)
Companion PR #24814 adds a dedicated
SingleEventTest.javaGatling harness (a fourthSierra Leone demo program, "Inpatient morbidity and mortality," using a ~14,000-option
ICD-10 diagnosis option set) specifically to exercise this code path, and dispatched
performance-tests-compare.ymlagainst this PR directly: baselinedhis2/core-dev:latest(unpatched master) vs candidate
dhis2/core-pr:24815(this branch).SMOKE profile (1 user, 10 sequential single-event imports):
LOAD profile (20 concurrent users, 50 imports each — real concurrent single-event
traffic, matching how single-event programs are actually imported in production: one
event per request, from many independent sessions):
That's a ~48x improvement in p95 under load, and — independent of the latency number —
the fix eliminates outright request failures under concurrency, not just slowness.
Both runs are unconfounded:
SingleEventTest.javais fully self-contained and shares noconfiguration with any other scenario, so these numbers measure exactly what they claim to.
Testing
TrackerPreheat,OptionValueSupplier, andValidationUtils's three validator call sites, including multi-text andprogram-rule-assign coverage — all ported and adapted from perf: stop Tracker preheat from fully materializing OptionSet.options [41] #24813.
EventImportValidationTest,TrackedEntityImportValidationTest,EnrollmentAttrValidationTest,ProgramRuleAssignActionTest) re-run after every behavioral change,confirming no regression to
E1125semantics.TrackerEvent/Enrollmentobjects built to re-resolve ASSIGN-actionvalues were missing their now-mandatory
.event(UID)/.enrollment(UID)identifiers, which compiled fine (Lombok's
@NonNullis runtime-only)but NPE'd at import time. Fixed before opening this PR.
spotless:applyrun on the full diff.java:S5786, redundantpublicon aJUnit5
@BeforeEachmethod) was fixed in a follow-up commit.SingleEventTest.java— see also comment on TrackerTest below.🤖 AI Assisted