Conversation
PR Reviewer Guide 🔍(Review updated until commit 9f3945c)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to d0bd7e9 Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit 38dfa20
Suggestions up to commit d553f91
Suggestions up to commit 4c2d3a6
Suggestions up to commit af1da74
Suggestions up to commit 58fc3f9
|
22a208d to
70fb145
Compare
|
Persistent review updated to latest commit 70fb145 |
|
Persistent review updated to latest commit 58fc3f9 |
58fc3f9 to
af1da74
Compare
|
Persistent review updated to latest commit af1da74 |
af1da74 to
4c2d3a6
Compare
|
Persistent review updated to latest commit 4c2d3a6 |
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (63.28%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #5807 +/- ##
============================================
+ Coverage 63.23% 63.28% +0.05%
- Complexity 8823 8852 +29
============================================
Files 938 939 +1
Lines 40236 40313 +77
Branches 4537 4550 +13
============================================
+ Hits 25445 25514 +69
- Misses 13966 13970 +4
- Partials 825 829 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4c2d3a6 to
d553f91
Compare
|
Persistent review updated to latest commit d553f91 |
|
Persistent review updated to latest commit 38dfa20 |
OpenSearch answers a search with HTTP 200 as long as one shard responded (search.default_allow_partial_results defaults to true), so a result can omit whole shards. OpenSearchResponse read only getHits()/getAggregations() and discarded the _shards block, so PPL presented the surviving shards' rows as the whole answer: no error, no warning. During a load spike, a WLM rejection or a node drop silently undercounts, e.g. a count of 750 where the answer is 990. DSL clients surface this from _shards; PPL had nowhere to put it. Capture the shard outcome and attach a PARTIAL_RESULT_SHARD_FAILURE warning through the existing warnings channel when the search did not cover every shard. - ShardStats records total/successful/skipped/failed/timed_out plus the distinct per-shard failure reasons, and builds the warning so both message and detail read identically wherever a scan raises it. - Detect all three partial shapes, not just failures: a failed shard, a shard with no available copy (successful + skipped < total, where failed is 0 -- the node-drop shape a failed-only check misses), and a timeout. - Record from BackgroundSearchScanner on the query thread. The search itself may run on the background pool, whose thread the warning sink cannot see. Scoped to the Calcite path, matching where the warnings channel is wired today; the V2 fallback and a setting to reject partial results instead of warning are left for follow-ups. Signed-off-by: Kai Huang <ahkcs@amazon.com>
The warnings array has been part of the PPL JSON response since opensearch-project#5657 but was never described in the protocol reference. Document both types, when the field is present, and which formats carry it. Signed-off-by: Kai Huang <ahkcs@amazon.com>
Pruning probes each index with field caps and an index filter, keeping the ones that come back. An index whose shards cannot be read does not come back, and field caps reports no failure for it -- it is simply absent from the response, indistinguishable from an index proven to hold nothing in the time range. Pruning therefore dropped it from the index expression. That loses data with nothing left to report. The search that runs afterwards covers every shard it was given, so its shard counts are complete, the PARTIAL_RESULT_SHARD_FAILURE warning has nothing to raise, and even search.default_allow_partial_results=false sees a whole search and permits it. Measured on a two-node cluster over a 4-shard pattern of 990 documents with one node stopped: stats count() returned 750 with no warning and no error, while the equivalent DSL search over the same pattern was rejected outright. Keep any index the probe cannot have ruled out: those absent from a second, unfiltered probe. A readable index reports its field caps whatever the time range, so absence there means the index could not be read at all. That holds regardless of what the cluster state believes, which also covers the window after a node stops but before the cluster manager marks its shards unassigned -- seconds, and precisely when a dashboard refresh would otherwise lose the index. Judged by a field caps probe rather than by reading the routing table on purpose: a cluster state request needs cluster:monitor/state, which an index-scoped role does not carry, so it would both disable pruning for those users and log a missing-privileges audit event on every query. The cost is that an index readable through its other shards while one shard is unassigned still looks prunable; its in-range documents would have to live only on the missing shard, which takes custom routing to arrange. Both probes run only when pruning would actually narrow the expression, so the healthy path is unchanged. When nothing matched the filter, pruning already declines, which keeps a partial answer from becoming an all-shards-failed error. Signed-off-by: Kai Huang <ahkcs@amazon.com>
The integTestWithSecurity suite runs only org.opensearch.sql.security.*, so a path whose IT lives elsewhere is never exercised with security installed. Two things about this change can only break there, and both did during review: - The warning travels the response channel that opensearch-project#5739 showed the security transport interceptor can drop on the transport-to-worker handoff. - Deciding whether an index was readable has to use an API the caller's role already allows. Reading the routing table needs cluster:monitor/state, which an index-scoped role does not carry, so it disabled pruning for exactly these users and logged a missing-privileges audit event on every query. The fixture holds three indices in one pattern -- one in range, one out of range carrying a field of its own, and one whose shard can never be allocated -- so a bounded query must prune the out-of-range index while keeping the unreadable one: legacy_only stops resolving, and the response still carries the warning. The unallocatable index is dropped between tests so no other class in the suite inherits a red cluster. Signed-off-by: Kai Huang <ahkcs@amazon.com>
38dfa20 to
d0bd7e9
Compare
|
Persistent review updated to latest commit d0bd7e9 |
Three holes from self-review, each a way the warning was still lost: - A PIT created over only some of its shards pins that subset, so every search against it looks complete and the gap is visible only at creation. Report it there. Creation itself now allows partial, matching the search path: with it forbidden, one unassigned shard turned any cursor or >max_result_window query into a 500, which keeping unreadable indices in the expression would have widened from unbounded queries to bounded ones. - buildResultSet drained warnings after the timewrap block, whose finally clears the same thread-local, so every timewrap query dropped them. Drain first. - describeFailures keyed dedup on "[index][shard] reason", so one cause spanning four shards printed four times and truncated to three. Key on the reason and label it with the first shard that hit it. The test only passed because it reused one shard id. Also: missing() no longer subtracts skipped twice, comments cut to what the code does not already say, the duplicated bounds helper lifted into SecurityTestBase, and the @after index drops guarded so a failed init reports its own cause. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit 9f3945c |
This branch has not been deployed
1 participant
Description
Two defects, both of which have to be fixed for a partial PPL result to be visible at all:
OpenSearchResponseread onlygetHits()/getAggregations()and threw away_shards, so a result assembled from the shards that answered was returned as if it were whole ([BUG] PPL returns HTTP 200 with silently partial results when shards fail (no _shards failure surfaced) #5687)._field_caps, which reports no failure for it, so pruning dropped it from the query entirely — leaving nothing for fix 1 (or forallow_partial_search_results=false) to detect ([BUG] Index pruning silently drops an index it cannot reach, hiding missing shards #5808, found while verifying fix 1).Fix 1 is the first commit, fix 2 the third. Without fix 2, fix 1 is inert on exactly the path that matters: every PPL query Dashboards issues carries request-level time bounds, which is what activates pruning.
OpenSearch answers a search with HTTP 200 as long as one shard responded —
search.default_allow_partial_resultsdefaults totrue, and the plugin never setsallowPartialSearchResultson the request, so every PPL search inherits it.OpenSearchResponsethen read onlygetHits()/getAggregations()and discarded the_shardsblock, so PPL presented the surviving shards' rows as the whole answer: no error, no warning, no way for the caller to tell.During a load spike (workload-management rejection, circuit breaker) or a node drop, that silently undercounts. Reproduced locally on a two-node cluster over a 4-shard pattern holding 990 documents:
DSL clients surface this from
_shards(OpenSearch Dashboards toasts "N of M shards failed" on the DQL path). PPL responses had nowhere to carry it.This captures the shard outcome and attaches a warning through the
Warning{type,message,detail}→QueryResponse.warnings→SimpleJsonResponseFormatterchannel added in #5657, so the existing Explore warning banner renders it:Two details worth reviewer attention:
The shard counters are not disjoint, which the completeness test has to account for. A shard skipped by the can-match phase is reported in both
skippedandsuccessful, so the invariant istotal = successful + failed + missing. Subtractingskippeda second time hides a missing shard behind every skipped one — verified on a live cluster, where one unassigned shard plus one skipped and one searched reportstotal: 3, successful: 2, skipped: 1. That matters because can-match skipping is the norm for a time-filtered query once a pattern exceedspre_filter_shard_size(default 128), i.e. exactly the dashboard workload this is for. Below that threshold the phase does not run, so neither a local cluster nor an IT can reproduce it;ShardStatsTestpins the arithmetic to the measured numbers instead.All three partial shapes are detected, not only failures. A missing shard — no copy available, the node-drop case — reports
failed: 0and simply does not add up (successful + skipped < total). CheckingfailedShards > 0alone, as the issue originally proposed, misses the most common production shape; OpenSearch Dashboards' own DQL shard-failure toast has the same blind spot. Atimed_outresponse is the third shape.The warning is raised on the query thread, not in the response constructor. In the Calcite path
client.searchruns on thesql-backgroundpool viaBackgroundSearchScanner, whose thread the warning thread-local cannot reach.fetchNextBatchruns on the query thread — the same one that later drains warnings inbuildResultSet— so the stats travel on the response object and are recorded there. Identical stats repeated across pages collapse to one warning via the channel's existing de-duplication.Testing
The integration test fixture is an index whose shard can never be allocated (an allocation filter naming a node that does not exist), alongside a healthy index in the same pattern. That yields
total: 2, successful: 1, failed: 0deterministically, with no node manipulation, and exercises the harder of the two detection paths. The unassignable index is dropped between tests so no other class inherits a red cluster.Verified locally: 9 new
ShardStatsTestcases, 3 newBackgroundSearchScannerTestcases, 4 new ITs, 35IndexPrunerTestcases, plus:opensearch:test(1747),:core:test(5286),CalcitePartialResultOnMappingConflictIT(10) andCalciteTimeBoundsPruningIT(10) green. A full:integ-test:integTestrun is in progress; I will post the result rather than assume it. Also confirmed end to end against a deployed build: the same query returns 990 with both nodes up and 750 with one stopped, now carrying the warning instead of presenting 750 as the answer.Defect 2 in detail (#5808): pruning silently dropped the evidence
Verifying the warning through the Dashboards path exposed a worse bug in index pruning (#5766, mine), so the second commit fixes it here rather than leaving the first commit ineffective where it matters most.
Pruning probes each index with field caps plus an
index_filterand keeps whichever come back. An index whose primary is unassigned cannot be probed, and field caps reports no failure for it — it is simply absent from the response, indistinguishable from an index proven to hold nothing in range:So the unreachable index was pruned out of the expression. The search that then ran covered every shard it was given, which means:
search.default_allow_partial_results=falsesaw a whole search and allowed it.Measured on a two-node cluster over a 4-shard pattern of 990 documents, one node stopped, with the request-level time bounds Dashboards sends:
750, no warning, no errorallow_partial_search_results=false750, still no errorThe fix keeps any index the probe could not have ruled out. The mechanism turns on a distinction one probe cannot make but two can — the second being the same
_field_capscall with no index filter:The unfiltered probe reaches nothing the filtered one could not; both miss an unreachable index identically. What it adds is a control that says what an absence means. Measured on a fixture carrying all three cases at once — one index in range, one healthy but out of range, one whose shard is unassigned:
shard_sec_recent(in range)shard_sec_old(healthy, out of range)shard_sec_unreadable(shard unassigned)The two absences in the filtered column look identical; the unfiltered column separates them.
shard_sec_oldproves it was reachable by appearing without the filter, so its absence with the filter is real evidence of no matching data.shard_sec_unreadablenever appears at all, so its absence is evidence of nothing — and that silence is what pruning was previously treating as proof of emptiness.In code:
unsearchable = resolved − unfiltered, thenkeep = candidates ∪ unsearchable. On the fixture above that prunes onlyshard_sec_old, which is exactly right.Two consequences, both intentional. An index that is readable but does not map the time field is also absent from the unfiltered probe, so it is kept — one extra shard in the search, never a wrong answer. And "unreadable" is inferred from absence rather than from a failure marker because
_field_capsoffers none:failed_indices: null,failures: null, which is the underlying API gap.Judged by a probe rather than by reading the routing table deliberately — see Security coverage below. Both probes run only when pruning would actually narrow the expression, so the healthy path keeps its single probe. And because pruning already declines when nothing matched the filter, keeping an unreachable index can never narrow a query down to only unreachable indices and turn a partial answer into an all-shards-failed error.
After the fix the three rows above read:
750plus the warning; rejected underallow_partial_search_results=false; and the DSL 503 unchanged.Security coverage
integTestWithSecurityruns onlyorg.opensearch.sql.security.*, so a path whose IT lives elsewhere is never exercised with the security plugin installed.ShardFailureWarningSecurityITcloses that for this change, and it is not hypothetical — the first version of the pruning fix read the routing table, which needscluster:monitor/state. An index-scoped role does not carry that, so the denied request made pruning decline for the whole query:TimeBoundsPruningSecurityITwent red, and every such user would have had pruning silently disabled plus a MISSING_PRIVILEGES audit event per query. Readability is now judged by a second field caps probe, an API those roles already allow, and a unit test asserts no cluster state request is ever issued.The fixture holds three indices in one pattern — one in range, one out of range carrying a field of its own, and one whose shard can never be allocated — so a bounded query must prune the out-of-range index while keeping the unreadable one:
source=shard_sec_* | stats count()PARTIAL_RESULT_SHARD_FAILURE(warning survives the transport handoff)PARTIAL_RESULT_SHARD_FAILURE(warning survives pruning)source=shard_sec_* | fields legacy_only, with boundsField [legacy_only] not found.(pruning still narrows)source=shard_sec_recent | stats count()All four verified against a live cluster; the security dimension itself is verified by CI, since the security suite cannot run on my machine (the
opensearch-securitybundle trips jar hell against this distro snapshot'slib/jackson).Scope
Calcite path only, matching where the warnings channel is wired today (
plugins.calcite.enableddefaults to true, so this is the primary path). Deliberately left for follow-ups:QueryResponsewithout warnings;allow_partial_search_results=falsenow correctly fails the query, but the failure arrives as an HTTP 500java.sql.SQLException ... the background task failed or interruptedrather than core's HTTP 503 "Search rejected due to missing shards". Worth mapping properly, and orthogonal to this change;allow_partial_search_results— it would be oneallowPartialSearchResults(false)call on the request, letting core produce its own error rather than inventing one.The warning
typeis deliberately distinct fromPARTIAL_RESULT: that type means "the engine chose to narrow to a subset of indices", and OpenSearch Dashboards gates a "Rerun without partial results" action on it, which reruns withpartial_result: falseand would do nothing for a shard failure. Happy to fold it intoPARTIAL_RESULT(a one-constant change) if we would rather widen that flag's meaning.Related Issues
Resolves #5687
Resolves #5808
Check List
docs/user/ppl/interfaces/protocol.mdgains a Warnings section, which also fills in thePARTIAL_RESULTtype Add an opt-in partial-result mode for aggregations on text/keyword mapping conflicts #5657 never documented).warningsarray already exists in the response contract.--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.