Repository navigation
Improve indexing - #26154
Improve indexing#26154
Conversation
…the existing reindexing atomic counters to Prometheus so operators can alert on failures, latency spikes, and backpressure without relying solely on database-flushed stats.
- Add ReindexingMetrics singleton (initialize/getInstance pattern matching
CacheMetrics) with job lifecycle counters, stage success/failed/warnings
counters, bulk request timers with SLA buckets, payload size distribution,
backpressure and promotion counters, and active/pending gauges
- Register in MicrometerBundle after StreamableLogsMetrics
- Instrument ReindexingOrchestrator.run() with job started/completed/failed/stopped
- Bridge StageStatsTracker.flush() deltas to Prometheus per stage and entity type
- Add bulk request latency timer and payload size recording in OpenSearchBulkSink
- Record backpressure events in SearchIndexExecutor.handleBackpressure()
- Record promotion success/failure in DefaultRecreateHandler
- Add ReindexingMetricsTest with 24 tests covering all metric types
github-actions
Bot
added
backend
safe to test
|
TypeScript types have been updated based on the JSON schema changes in the PR |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| if (state.compareAndSet(State.HALF_OPEN, State.OPEN)) { | ||
| openedAt = now; | ||
| LOG.warn("Circuit breaker transitioning HALF_OPEN → OPEN (probe failed)"); | ||
| recordTransition("closed_to_open"); |
There was a problem hiding this comment.
⚠️ Bug: Circuit breaker records wrong transition metric on probe failure
In BulkCircuitBreaker.recordFailure(), when the circuit breaker transitions from HALF_OPEN → OPEN (probe request failed), the metric recorded via recordTransition("closed_to_open") at line 81 is incorrect. It should be "half_open_to_open".
The log message on line 80 correctly describes the transition as "HALF_OPEN → OPEN (probe failed)", but the metric tag passed to ReindexingMetrics.recordCircuitBreakerTrip() says "closed_to_open", which:
- Inflates the
closed_to_opentransition count with events that are actuallyhalf_open_to_open - Makes it impossible to distinguish between initial circuit opens and probe failures in monitoring dashboards
- Hides the probe failure rate, which is critical for diagnosing search cluster recovery issues
Suggested fix:
if (state.compareAndSet(State.HALF_OPEN, State.OPEN)) {
openedAt = now;
LOG.warn("Circuit breaker transitioning HALF_OPEN → OPEN (probe failed)");
recordTransition("half_open_to_open");
}
Was this helpful? React with 👍 / 👎
🔍 CI failure analysis for 6c3becd: playwright-ci-postgresql (5, 6) on commit 6c3becd failed with flaky frontend tests - browser crashes, element visibility/content assertion failures. Same pre-existing test infrastructure pattern. All failures remain unrelated to PR's search indexing refactoring.Issue 1: playwright-ci-postgresql (5, 6) - Flaky Frontend Tests (Latest Occurrence)The Root CauseErrors:
Affected tests:
DetailsThis failure is unrelated to the PR changes. The PR modifies Java backend search indexing architecture but does not modify frontend test files or UI components. The failures are characteristic of pre-existing flaky frontend tests:
Issue 2: Integration Tests - Vector Embedding Bugs (12+ Occurrences)Vector embedding failures continue on every commit. Pre-existing systemic issue unrelated to PR. Issues 3-5: Other Unrelated Failures
Summary: All failures are unrelated to the PR's search indexing architecture refactoring. Code Review
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
| } | ||
|
|
||
| @Override | ||
| public ExecutionResult execute(ReindexingConfiguration config, ReindexingJobContext context) { |
There was a problem hiding this comment.
⚠️ Bug: SearchIndexExecutor never closed: sink leak + missing finalize on failure
When executeSingleServer() throws (e.g., during reIndexFromStartToEnd), the catch in execute() returns a FAILED result but never calls closeSinkIfNeeded() or finalizeReindex(). The close() method at line 1924 has the correct cleanup logic (closing the sink, flushing stats, and finalizing reindex), but neither SingleServerIndexingStrategy nor ReindexingOrchestrator ever invokes it.
This causes two problems on failure paths:
-
Resource leak: The
BulkSink(ElasticSearch/OpenSearch client connections, internal thread pools, semaphores) is never closed. These resources accumulate across failed reindex attempts. -
Always-promote strategy not applied: The
cleanup()→finalizeReindex()chain that implements the "always-promote" strategy (promoting partial data from staged indices) is skipped. This directly contradicts the PR's stated design goal — partial data in staged indices is abandoned rather than promoted, and staged indices become orphaned.
The fix should ensure close() is called in a finally block or use try-with-resources on the executor.
Suggested fix:
// In SingleServerIndexingStrategy.execute():
@Override
public ExecutionResult execute(ReindexingConfiguration config, ReindexingJobContext context) {
try {
return executor.execute(config, context);
} finally {
executor.close();
}
}
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
| oldIndicesToDelete, | ||
| stagedIndex); | ||
|
|
||
| if (oldIndicesToDelete.contains(canonicalIndex)) { |
There was a problem hiding this comment.
⚠️ Edge Case: Index deletion before alias swap can leave entity unsearchable
Both finalizeReindex() (line 125-131) and promoteEntityIndex() (line 279-285) delete the canonical index before performing the atomic alias swap. If deleteIndexWithBackoff succeeds but the subsequent swapAliases call fails (e.g., due to a network issue or cluster timeout), the canonical index is already gone and the aliases aren't pointing to the new staged index.
The code at lines 136-141 / 290-298 returns early on swap failure without recovery, leaving the entity type completely unsearchable until the next successful reindex.
This edge case is partly inherent to the ES/OS limitation (can't have an index and alias with the same name), but can be mitigated:
- Before deleting the canonical index, verify the staged index is healthy
- Consider creating a temporary alias before deletion as a safety net
- At minimum, attempt to re-create the canonical index pointing to staged data if swap fails
In normal post-first-reindex operation this path shouldn't trigger (canonical name becomes an alias), but during initial setup or after manual index manipulation, it represents a data-loss risk.
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
|
|
Describe your changes:
Fixes
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
Resilience & Fault Tolerance:
Performance Optimization:
timeSeriesMaxDaysandtimeSeriesEntityDayslimits historical data reindexingObservability & Metrics:
Distributed Execution:
Load Testing Infrastructure:
This will update automatically on new commits.