-
Notifications
You must be signed in to change notification settings - Fork 216
fix(engine): fix ES indexing cursor skipping expectations with same timestamp (#6490) #6493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0ec87d7
391144f
e5f9f32
5e83a21
2bd91c4
b37e477
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| package io.openaev.migration; | ||
|
|
||
| import java.sql.Statement; | ||
| import org.flywaydb.core.api.migration.BaseJavaMigration; | ||
| import org.flywaydb.core.api.migration.Context; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
| @Component | ||
| public class V5_35__Add_indexing_last_id extends BaseJavaMigration { | ||
|
|
||
| @Override | ||
| public void migrate(Context context) throws Exception { | ||
| try (Statement statement = context.getConnection().createStatement()) { | ||
| statement.execute( | ||
| "ALTER TABLE indexing_status ADD COLUMN IF NOT EXISTS indexing_last_id TEXT NULL"); | ||
| // Force a one-time full reindex for expectations so that any rows that were previously | ||
| // skipped (same-timestamp batch overflow before this fix) are picked up on next cycle. | ||
| statement.execute( | ||
| "UPDATE indexing_status SET indexing_last_id = NULL, indexing_status_indexing_date = '1970-01-01 00:00:00'" | ||
| + " WHERE indexing_status_type = 'expectation-inject'"); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,4 +21,25 @@ public interface Handler<T extends EsBase> { | |
| * @return list data to index | ||
| */ | ||
| List<T> fetch(Instant from, int limit); | ||
|
|
||
| /** | ||
| * 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). | ||
|
Comment on lines
+25
to
+33
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| * | ||
| * @param from lower-bound timestamp (exclusive unless lastId is provided) | ||
| * @param lastId ID of the last successfully indexed entity at {@code from}; when non-null the | ||
| * query returns items at {@code from} with ID strictly greater than this value, plus all | ||
| * items strictly after {@code from} | ||
| * @param limit maximum number of records to fetch per batch | ||
| * @return list data to index | ||
| */ | ||
| default List<T> fetch(Instant from, String lastId, int limit) { | ||
| return fetch(from, limit); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: This query is really closed from the previous one. Can we do some refacto here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).