fix(engine): fix ES indexing cursor skipping expectations with same timestamp (#6490)#6493
fix(engine): fix ES indexing cursor skipping expectations with same timestamp (#6490)#6493Seb-MIGUEL wants to merge 6 commits into
Conversation
d8716d8 to
2d56f5f
Compare
There was a problem hiding this comment.
Pull request overview
Implements a compound keyset cursor for ES/OpenSearch indexing to prevent permanently skipping inject expectations when more than one full batch shares the same updated_at timestamp, keeping Elasticsearch/OpenSearch results consistent with the database.
Changes:
- Adds
indexing_last_idstorage to persist a secondary cursor (ID) alongsidelastIndexing. - Extends the
Handlercontract with a backward-compatiblefetch(from, lastId, limit)default method and implements it forInjectExpectationHandler. - Updates Elastic/OpenSearch bulk processing to pass/persist
lastIdand to clear it when the batch is not full.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| openaev-model/src/main/java/io/openaev/service/OpenSearchService.java | Passes compound cursor to handlers and persists lastId in IndexingStatus. |
| openaev-model/src/main/java/io/openaev/service/ElasticService.java | Same compound-cursor tracking logic for the Elasticsearch client. |
| openaev-model/src/main/java/io/openaev/engine/model/injectexpectation/InjectExpectationHandler.java | Implements compound-cursor fetch and refactors mapping into a helper. |
| openaev-model/src/main/java/io/openaev/engine/Handler.java | Adds default compound-cursor fetch(from, lastId, limit) API. |
| openaev-model/src/main/java/io/openaev/database/repository/InjectExpectationRepository.java | Adds findForIndexingAfter(from, lastId, limit) with compound cursor ordering. |
| openaev-model/src/main/java/io/openaev/database/model/IndexingStatus.java | Adds persisted lastId field used by the compound cursor. |
| openaev-api/src/main/java/io/openaev/migration/V5_35__Add_indexing_last_id.java | Flyway migration adding indexing_last_id column. |
| SELECT ie.inject_expectation_id FROM injects_expectations ie | ||
| JOIN injects i ON i.inject_id = ie.inject_id | ||
| WHERE i.inject_updated_at > :from |
There was a problem hiding this comment.
Fixed: the UNION branch for \i.inject_updated_at\ now uses the compound cursor: \WHERE i.inject_updated_at > :from OR (i.inject_updated_at = :from AND ie.inject_expectation_id > :lastId).
| SELECT ie.inject_expectation_id FROM injects_expectations ie | ||
| JOIN injects i ON i.inject_id = ie.inject_id | ||
| JOIN injectors_contracts ic ON ic.injector_contract_id = i.inject_injector_contract | ||
| WHERE ic.injector_contract_updated_at > :from |
There was a problem hiding this comment.
Fixed: same compound cursor applied to the injector-contract UNION branch: WHERE ic.injector_contract_updated_at > :from OR (ic.injector_contract_updated_at = :from AND ie.inject_expectation_id > :lastId).
| /** | ||
| * Variant of {@link #fetch(Instant, int)} using a compound keyset cursor (timestamp + last entity | ||
| * ID) to avoid missing items that share the same {@code updated_at} timestamp when the previous | ||
| * batch was full. | ||
| * | ||
| * <p>The default implementation ignores {@code lastId} and falls back to the basic cursor, | ||
| * providing backward-compatible behaviour for handlers that do not need compound pagination. | ||
| * Override in handlers where the same-millisecond duplicate issue is observable (e.g. | ||
| * InjectExpectationHandler). |
There was a problem hiding this comment.
Added integration test compound_cursor_returns_all_expectations_at_same_timestamp in IndexingRegressionTest.java (nested class InjectExpectationIndexing). Creates 3 expectations forced to the same updated_at, verifies fetch(FROM, 2) returns first 2, then fetch(FROM, lastId, 2) returns the 3rd proving no expectation is skipped.
b116ffd to
caf91fb
Compare
…ith same timestamp When multiple InjectExpectation entities are saved in the same batch (e.g. during expiration of many expectations), Hibernate sets the same @UpdateTimestamp millisecond for all of them. The existing polling query uses a strict > comparison on updated_at, so when the indexing batch size (500) cuts through a set of items sharing the same timestamp, the remaining items are permanently skipped. Fix: introduce a compound keyset cursor (updated_at, inject_expectation_id). - Add indexing_last_id column to indexing_status (V5_35 migration) - Add findForIndexingAfter query with compound cursor in repository - Add fetch(from, lastId, limit) default method to Handler interface; InjectExpectationHandler overrides it with the compound-cursor query - OpenSearchService and ElasticService track lastId per indexing cycle: when a full batch is returned, lastId is persisted so the next cycle continues from the exact item boundary; otherwise lastId is cleared This guarantees all expectations are eventually indexed regardless of how many share the same updated_at timestamp, fixing the inconsistency between the dashboard detection percentage (ES) and the inject detail page (DB).
caf91fb to
0ec87d7
Compare
| // Act — first batch (limit=2): returns 2 expectations ordered by (updated_at, id) | ||
| List<EsInjectExpectation> batch1 = injectExpectationHandler.fetch(FROM, 2); | ||
| assertThat(batch1).hasSize(2); | ||
|
|
||
| // Act — compound cursor continues from last item of batch1; must return the 3rd | ||
| String lastId = batch1.getLast().getBase_id(); | ||
| List<EsInjectExpectation> batch2 = injectExpectationHandler.fetch(FROM, lastId, 2); |
There was a problem hiding this comment.
Fixed: the second batch now uses lastUpdatedAt = batch1.getLast().getBase_updated_at() as the from parameter, which mirrors how the production indexing job updates lastIndexing after a full batch. With from = sharedTimestamp and lastId = exp2.id, only exp3 satisfies (updated_at = sharedTimestamp AND id > exp2.id), correctly proving the compound cursor skips nothing.
| try (Statement statement = context.getConnection().createStatement()) { | ||
| statement.execute( | ||
| "ALTER TABLE indexing_status ADD COLUMN IF NOT EXISTS indexing_last_id TEXT NULL"); | ||
| } |
There was a problem hiding this comment.
Fixed: the migration now also resets the indexing cursor for expectation-inject (sets indexing_last = epoch and indexing_last_id = NULL) so that the next indexing cycle re-processes all expectations from the beginning, picking up any rows that were previously skipped due to the same-timestamp overflow.
…eset indexing on migration
|
Hey, |
…match main branch
|
Hi @RomuDeuxfois! The one-batch-per-invocation design is intentional for three reasons:
The compound cursor fix in this PR ensures correctness at batch boundaries: when more than one full batch shares the same updated_at, successive scheduled invocations now correctly page through them with (updated_at = :from AND id > :lastId) rather than skipping them. |
| LIMIT :limit | ||
| """, | ||
| nativeQuery = true) | ||
| List<RawInjectExpectationIndexing> findForIndexingAfter( |
There was a problem hiding this comment.
todo: This query is really closed from the previous one. Can we do some refacto here ?
There was a problem hiding this comment.
Done - merged findForIndexing and findForIndexingAfter into a single query. The compound-cursor clause is now guarded by :lastId IS NOT NULL, so passing null gives the simple > :from behaviour (first batch) and passing a non-null ID gives the full compound cursor. Removed 70 lines of duplicated SQL. InjectExpectationHandler.fetch(from, limit) now delegates to findForIndexingAfter(from, null, limit) (commit 5e83a21).
Problem
The custom dashboard shows 100% detected for some injects. Clicking opens a drawer with
inject_expectation_status = SUCCESSfrom Elasticsearch. Clicking an inject navigates to the inject detail page which shows 100% not detected — inconsistent data between ES and DB.Fixes #6490
Root Cause
findForIndexinguses a strict>comparison onupdated_at:When
bulkComputeTechnicalExpectationsexpires hundreds of expectations in onesaveAll()call, Hibernate's@UpdateTimestampassigns the same millisecond to all of them. If there are >500 with the same timestamp, the cursor advances toT, thenupdated_at > Tpermanently skips the rest.Fix: Compound keyset cursor
Add a secondary
inject_expectation_idcursor so the query can resume within a timestamp:Changes
V5_35__Add_indexing_last_id.javaindexing_last_id TEXT NULLtoindexing_statusIndexingStatus.javalastIdfield for compound cursorInjectExpectationRepository.javafindForIndexingAfter(from, lastId, limit)queryHandler.javafetch(from, lastId, limit)method (backward-compatible)InjectExpectationHandler.javamap()helperOpenSearchService.javalastIdinbulkProcessing, pass to handlerElasticService.javaCursor logic
lastId = lastItem.id, keep samelastIndexinglastId = null, advancelastIndexingas before