Fixes #32255: Surface search shard failures instead of a silent empty result - #32401
Conversation
… result Both engines answer a search whose shards threw with HTTP 200 and whatever the surviving shards produced. Nothing in the service looked at _shards.failed on any read path -- 14 client.search() sites per engine, zero shards() references -- so a broken query engine and a genuinely empty catalog were indistinguishable, with no WARN or ERROR logged either. The shards that throw are the ones holding data (an empty index has nothing to score and always succeeds), so the hits that failed to come back are exactly the ones the user was looking for. On OpenSearch 3.3.2 that turns every search into an ordinary "no results" screen. Guard the client rather than each caller: search(SearchRequest, Class) is non-final on both vendor clients and each engine builds its client at exactly one site, so one subclass per engine covers every search in the service. The final builder-lambda overload compiles to a call on the overridable one, so it is covered too. Shard failures are always logged with the reason the engine gave. Only a response that lost shards and carries no hits is rejected -- one that lost shards and still found something is degraded rather than wrong, and failing it would turn every rolling restart into a user-visible outage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
backend
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR hardens OpenMetadata’s search layer by detecting Elasticsearch/OpenSearch partial shard failures (_shards.failed > 0) and preventing those failures from being silently surfaced to callers as an ordinary empty result set (notably the #32255 failure mode).
Changes:
- Add a shared
SearchShardFailurespolicy that WARN-logs shard failures and throwsSearchExceptionwhen shard failures coincide with zero hits. - Wrap both vendor clients with shard-failure-aware subclasses so all
client.search(...)call sites are covered without per-caller edits. - Add unit tests validating behavior for healthy, degraded-with-hits, and degraded-with-no-hits responses (including the builder-lambda overload).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/search/SearchShardFailures.java | Implements the shared policy for logging and rejecting untrustworthy empty results on shard failures. |
| openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/ShardFailureAwareOpenSearchClient.java | Overrides OpenSearch search(...) to enforce SearchShardFailures on every search response. |
| openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchClient.java | Switches OpenSearch client construction to the shard-failure-aware subclass. |
| openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/ShardFailureAwareElasticsearchClient.java | Overrides Elasticsearch search(...) to enforce SearchShardFailures on every search response. |
| openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/ElasticSearchClient.java | Switches Elasticsearch client construction to the shard-failure-aware subclass. |
| openmetadata-service/src/test/java/org/openmetadata/service/search/SearchShardFailuresTest.java | Unit tests for the shared shard-failure policy. |
| openmetadata-service/src/test/java/org/openmetadata/service/search/opensearch/ShardFailureAwareOpenSearchClientTest.java | Unit tests ensuring OpenSearch client wrapper guards both overloads and handles missing totals. |
| openmetadata-service/src/test/java/org/openmetadata/service/search/elasticsearch/ShardFailureAwareElasticsearchClientTest.java | Unit tests ensuring Elasticsearch client wrapper guards both overloads and handles missing totals. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| String failures = String.join("; ", listOrEmpty(failureDetails)); | ||
| LOG.warn( | ||
| "Search completed with {} of {} shards failing, hits={}. Shard failures: {}", | ||
| failedShards, | ||
| totalShards, | ||
| hits, | ||
| failures.isEmpty() ? "<none reported>" : failures); | ||
|
|
||
| if (hits == 0) { | ||
| throw new SearchException( | ||
| String.format( | ||
| "Search failed on %d of %d shards and returned no results, so an empty result cannot " | ||
| + "be trusted. Shard failures: %s", | ||
| failedShards, totalShards, failures.isEmpty() ? "<none reported>" : failures)); | ||
| } |
✅ Playwright Results — workflow succeededValidated commit ✅ 4483 passed · ❌ 0 failed · 🟡 5 flaky · ⏭️ 1 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 1h 0m 41s ⏱️ Max setup 4m 30s · max shard execution 21m 9s · max shard-job elapsed before upload 24m 38s · reporting 18s 🌐 217.81 requests/attempt · 2.31 app boots/UI scenario · 45.86% common-shard skew Optimization targets still in progress:
🟡 5 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
# Conflicts: # openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/ElasticSearchClient.java
Code Review ✅ Approved 2 resolved / 2 findingsSurfaces search shard failures instead of silent empty results by adding a guard at the client level that covers all search call sites. Resolved two edge cases: hits==0 guard misfiring on aggregation/size:0 searches, and deep-paging with track_total_hits off throwing false 500s. Policy: failed shards always log a WARN with the reason and failing index; searches losing shards with zero hits return 500; degraded searches that still found rows pass through. Comprehensive test coverage (14 tests, 100% on SearchShardFailures) and 2250 regression tests all passing. ✅ 2 resolved✅ Edge Case: hits==0 guard misfires on aggregation/size:0 searches
✅ Edge Case: Deep-paging with track_total_hits off can throw a false 500
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
Describe your changes:
Fixes #32255
I worked on surfacing search shard failures because both engines answer a search whose shards threw with HTTP 200 and whatever the surviving shards produced, and nothing in the service ever looked at
_shards.failed— 14client.search()sites per engine, zeroshards()references, no WARN or ERROR logged. The shards that throw are the ones holding data (an empty index has nothing to score and always succeeds), so the hits that failed to come back are exactly the ones the user was looking for, and a broken search engine renders as an ordinary "no results" screen.Type of change:
High-level design:
Where the guard lives.
search(SearchRequest, Class)is non-final on both vendor clients, and each engine builds its client at exactly one site (OpenSearchClient:172,ElasticSearchClient:185). One subclass per engine therefore covers everyclient.search(...)in the service — search, aggregation, column and lineage managers alike — with two one-line swaps and no call-site edits. The builder-lambda overload isfinal, but it compiles to a call on the overridable one, so it is covered too; that is an assumption about someone else's bytecode, so it is asserted in a test rather than reasoned about.msearch/count/scrollare not used anywhere in the service, sosearch()is the whole read surface.Rejected alternative: guarding each caller. That is 28 edits across two files, leaves the aggregation/lineage managers untouched, and leaves call site #29 unguarded.
Policy (
SearchShardFailures, shared by both engines):failed > 0→ always WARN, with the reason the engine gave and the failing index (table_search_index[0]: null_pointer_exception), not just a shard count. The issue reports this single line as what would have saved a day of debugging.failed > 0 && hits == 0→SearchException(500). A search that lost shards and found nothing cannot be shown to anyone as "no results".failed > 0 && hits > 0→ passes through unchanged. Degraded is not wrong, and failing it would turn every rolling restart and shard relocation into a user-visible outage.hitsreadshits.total.value, falling back to the returned page whentrack_total_hitsis off (it is caller-controlled viaSearchResource) — otherwise an absent total reads as zero and would fail every degraded search that did return rows.Backward compatibility: the only behaviour change is that one previously-silent case now returns 500 instead of a misleading 200. Healthy and degraded-but-productive searches are byte-identical.
Out of scope: the trigger on OpenSearch 3.3.2 is an upstream Lucene 10.3.1 bug, fixed in 3.4.0 / Lucene 10.3.2. This PR makes the failure loud; it does not make 3.3.2 work. The documented OpenSearch floor should move to 3.4.0 (this repo already standardizes on 3.4.0 in every compose file,
TestSuiteBootstrap, and all ITs), but the docs live inopenmetadata-docsand are not touched here.Tests:
Use cases covered
track_total_hits, and via bothsearch(...)overloads, on both OpenSearch and Elasticsearch.Unit tests
openmetadata-service/src/test/java/org/openmetadata/service/search/SearchShardFailuresTest.javaopenmetadata-service/src/test/java/org/openmetadata/service/search/opensearch/ShardFailureAwareOpenSearchClientTest.javaopenmetadata-service/src/test/java/org/openmetadata/service/search/elasticsearch/ShardFailureAwareElasticsearchClientTest.javamvn jacoco:prepare-agent test jacoco:report -pl openmetadata-service:mvn test -pl openmetadata-service -Dtest='org.openmetadata.service.search.**'→ 2250 tests, 0 failures.mvn spotless:checkclean.Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
Reproduced the reporter's deployment shape end to end on a real cluster, using OpenMetadata's own shipped default ranking query (4
constant_score+ 4script_scorestages under adis_max):docker run -d -p 9333:9200 -e discovery.type=single-node -e DISABLE_SECURITY_PLUGIN=true opensearchproject/opensearch:3.3.2table_search_index, 1 doc) plus three empty ones, all behind adataAssetalias.Only the populated index failed — the exact signature in the issue. That response is what the new guard rejects, and the unit tests assert on the same shape.
While reproducing this I found the issue's stated mechanism is not quite right, and the correction matters for anyone triaging it: the NPE is not caused by having two or more
script_scoreclauses. It fires when a singlescript_scoreclause matches zero documents while sitting in a disjunction next to any clause that does match. Two matchingscript_scoreclauses are fine; a lone non-matching one is fine. Verified on 3.3.2 (Lucene 10.3.1) against 3.4.0 (Lucene 10.3.2) as a control. This explains the three things the report could not: why only populated indices fail, why removing one ranking rule did not help but removing all four did, and whysize: 0gives a false negative. It also means no ranking-rule trimming is a reliable workaround, which is why this PR does not attempt a query-shape fix.UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.Bug fix
#32255is referenced in the test and inSearchShardFailuresfor future reference.🤖 Generated with Claude Code