Conversation
github-actions
Bot
added
backend
safe to test
There was a problem hiding this comment.
Pull request overview
This PR fixes a flaky/failing unit test setup for request latency metrics by avoiding concurrent modification of Micrometer’s global registry during test initialization.
Changes:
- Update
@BeforeEachregistry cleanup to iterate over a snapshot copy ofMetrics.globalRegistry.getRegistries()before removing registries.
| b.create( | ||
| c -> | ||
| c.index(indexName) |
There was a problem hiding this comment.
createEntities is documented/used as a create-or-update bulk upsert, but this change switches the bulk op to create, which will skip existing document IDs (409) instead of updating them. This can leave the search index stale for normal re-indexing flows (e.g., SearchRepository.createEntitiesIndex repeatedly indexes the same entity IDs). Consider using index (or update with docAsUpsert) here to preserve upsert semantics; if the intent is truly create-only, the method name/JavaDoc and call sites should be adjusted accordingly.
| b.create( | |
| c -> | |
| c.index(indexName) | |
| b.index( | |
| i -> | |
| i.index(indexName) |
| if (response.errors()) { | ||
| LOG.error( | ||
| "Bulk indexing to ElasticSearch encountered errors. Index: {}, Total: {}, Failed: {}", | ||
| indexName, | ||
| docsAndIds.size(), | ||
| response.items().stream().filter(item -> item.error() != null).count()); | ||
| long realFailures = | ||
| response.items().stream() | ||
| .filter(item -> item.error() != null && item.status() != 409) | ||
| .count(); | ||
|
|
||
| if (realFailures > 0) { | ||
| LOG.error( | ||
| "Bulk indexing to ElasticSearch encountered errors. Index: {}, Total: {}, Failed: {}", | ||
| indexName, | ||
| docsAndIds.size(), | ||
| realFailures); | ||
| } | ||
|
|
||
| response.items().stream() | ||
| .filter(item -> item.error() != null) | ||
| .forEach( | ||
| item -> { | ||
| if (SearchIndexRetryQueue.isUuid(item.id())) { | ||
| SearchIndexRetryQueue.enqueue( | ||
| item.id(), | ||
| null, | ||
| SearchIndexRetryQueue.failureReason( | ||
| "createEntitiesItemError", | ||
| new RuntimeException(item.error().reason()))); | ||
| if (item.status() == 409) { | ||
| LOG.debug("Document already exists for ID {}, skipping create", item.id()); | ||
| } else { |
There was a problem hiding this comment.
When response.errors() is true solely due to 409 conflicts, this branch currently emits no info/warn-level summary (and only debug per-item logs). That makes bulk operations appear to do nothing at default log levels even though documents were skipped. Consider logging a summary count for conflicts (e.g., created vs already-existed) or treating conflict-only responses as a non-error path.
| b.create( | ||
| c -> | ||
| c.index(indexName) | ||
| .id(entry.getKey()) | ||
| .document(toJsonData(entry.getValue()))))); |
There was a problem hiding this comment.
createEntities is documented/used as a create-or-update bulk upsert, but switching the bulk op to create makes indexing idempotent only for new docs and will skip existing IDs with 409 instead of updating them. This risks stale search docs for normal indexing flows (e.g., SearchRepository.createEntitiesIndex re-indexes existing entities/columns). Consider using index (or update with docAsUpsert) to preserve upsert semantics; if create-only is intended, update the method contract and callers accordingly.
| if (response.errors()) { | ||
| LOG.error( | ||
| "Bulk indexing to OpenSearch encountered errors. Index: {}, Total: {}, Failed: {}", | ||
| indexName, | ||
| docsAndIds.size(), | ||
| response.items().stream().filter(item -> item.error() != null).count()); | ||
| long realFailures = | ||
| response.items().stream() | ||
| .filter(item -> item.error() != null && item.status() != 409) | ||
| .count(); | ||
|
|
||
| if (realFailures > 0) { | ||
| LOG.error( | ||
| "Bulk indexing to OpenSearch encountered errors. Index: {}, Total: {}, Failed: {}", | ||
| indexName, | ||
| docsAndIds.size(), | ||
| realFailures); | ||
| } | ||
|
|
||
| response.items().stream() | ||
| .filter(item -> item.error() != null) | ||
| .forEach( | ||
| item -> { | ||
| if (SearchIndexRetryQueue.isUuid(item.id())) { | ||
| SearchIndexRetryQueue.enqueue( | ||
| item.id(), | ||
| null, | ||
| SearchIndexRetryQueue.failureReason( | ||
| "createEntitiesItemError", | ||
| new RuntimeException(item.error().reason()))); | ||
| if (item.status() == 409) { | ||
| LOG.debug("Document already exists for ID {}, skipping create", item.id()); | ||
| } else { |
There was a problem hiding this comment.
When response.errors() is true only because of 409 conflicts, this path does not log any info/warn-level summary (only debug per-item logs). Consider logging a conflict summary (created vs already-existed) or treating conflict-only responses as a non-error to avoid silent skips at default log levels.
| new java.util.ArrayList<>(Metrics.globalRegistry.getRegistries()) | ||
| .forEach(Metrics.globalRegistry::remove); |
There was a problem hiding this comment.
For consistency/readability, prefer importing java.util.ArrayList and using new ArrayList<>(...) (as done in MetricsErrorHandlingTest in the same package) instead of using the fully-qualified new java.util.ArrayList<>(...) inline.
| * <p>Test isolation: Uses TestNamespace for unique entity naming Parallelization: Safe for | ||
| * concurrent execution via @Execution(ExecutionMode.CONCURRENT) |
There was a problem hiding this comment.
The class JavaDoc still states the test is safe for concurrent execution via @Execution(ExecutionMode.CONCURRENT), but the annotation is now SAME_THREAD. Please update the JavaDoc to reflect the new (non-concurrent) execution mode so future readers understand the constraint.
| * <p>Test isolation: Uses TestNamespace for unique entity naming Parallelization: Safe for | |
| * concurrent execution via @Execution(ExecutionMode.CONCURRENT) | |
| * <p>Test isolation: Uses TestNamespace for unique entity naming. | |
| * | |
| * <p>Parallelization: Runs with @Execution(ExecutionMode.SAME_THREAD) and is not intended for | |
| * concurrent execution. |
| b.index( | ||
| i -> | ||
| i.index(indexName) | ||
| b.create( |
There was a problem hiding this comment.
PR title/description focuses on fixing RequestLatency tests, but this PR also changes bulk indexing behavior for both Elasticsearch and OpenSearch and modifies an integration test’s execution mode/timeout. Please update the PR description/title (or split into separate PRs) so reviewers can assess these search/indexing behavior changes explicitly.
🔴 Playwright Results — 1 failure(s), 20 flaky✅ 3597 passed · ❌ 1 failed · 🟡 20 flaky · ⏭️ 207 skipped
Genuine Failures (failed on all attempts)❌
|
Code Review ✅ ApprovedFixes RequestLatencyTests with targeted corrections to test reliability. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Describe your changes:
Fixes
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
GlossaryOntologyExportITfromCONCURRENTtoSAME_THREADexecution to prevent thread pool exhaustion during synchronous Fuseki writesGlossaryOntologyExportITfrom 30 to 60 secondsConcurrentModificationExceptioninRequestLatencyContextTestby copying registry list before iterationThis will update automatically on new commits.