Warn when a cross-cluster search skips a remote cluster

[ahkcs]

Description

With skip_unavailable: true on a remote, a cross-cluster search that can't reach that remote skips it and still answers 200. The skipped cluster's shards are never collected, so _shards looks complete, and the skip shows up only in _clusters:

"_clusters": { "total": 6, "successful": 5, "skipped": 1 },
"_shards":   { "total": 90, "successful": 90, "failed": 0 }

The PARTIAL_RESULT warning from #5807 reads only _shards, so PPL returned five clusters' rows as the whole answer with no warning. This reads _clusters too and reports a skipped cluster like a missing shard:

"warnings": [{
  "type": "PARTIAL_RESULT",
  "message": "Results are partial: 1 of 6 clusters was skipped.",
  "detail": "_shards: total 90, successful 90, skipped 0, failed 0; _clusters: total 6, successful 5, skipped 1"
}]

Remote down, skip_unavailable	Before	After
false (default)	Search fails with an error	Same
true	Rows from the other clusters, no warning	Same rows, plus the warning

A shard failure still leads the message when both happen; the skipped cluster stays in the detail. _clusters only carries counts, so the warning can't name the cluster that was skipped.

Testing

• ShardStatsTest: one and several skipped clusters, a cross-cluster search that reached every cluster, and a shard failure together with a skipped cluster.
• CalcitePartialResultOnShardFailureIT: registers a remote whose only seed refuses connections, with skip_unavailable: true, and queries the local index plus that remote. It returns the local rows with the warning above.

Related Issues

Follow-up to #5807 (#5687).

Check List

• New functionality includes testing.
• New functionality has been documented. n/a, the warning type and format are unchanged.
• API changes companion pull request created, if applicable. n/a.
• Commits are signed per the DCO using --signoff.
• Public documentation issue/PR created, if applicable. n/a.

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.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null-safety for cluster counts Add null-safety checks for the cluster count methods. If clusters is non-null but any of its methods (getTotal(), getSuccessful(), getSkipped()) return null or throw exceptions, this could cause a NullPointerException or unexpected behavior. opensearch/src/main/java/org/opensearch/sql/opensearch/response/ShardStats.java [60-63] private static Clusters clustersOf(SearchResponse.Clusters clusters) { - return clusters == null - ? Clusters.NONE - : new Clusters(clusters.getTotal(), clusters.getSuccessful(), clusters.getSkipped()); + if (clusters == null) { + return Clusters.NONE; + } + int total = clusters.getTotal() != null ? clusters.getTotal() : 0; + int successful = clusters.getSuccessful() != null ? clusters.getSuccessful() : 0; + int skipped = clusters.getSkipped() != null ? clusters.getSkipped() : 0; + return new Clusters(total, successful, skipped); } Suggestion importance[1-10]: 2 __ Why: The suggestion adds unnecessary null checks for primitive int return types. Methods like getTotal(), getSuccessful(), and getSkipped() return primitive int values which cannot be null in Java. The existing null check for clusters itself is sufficient.	Low

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.68%. Comparing base (c640cb9) to head (a028055).
⚠️ Report is 5 commits behind head on main.

❌ Your project check has failed because the head coverage (63.68%) 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    #5851      +/-   ##
============================================
+ Coverage     63.64%   63.68%   +0.04%     
- Complexity     9017     9029      +12     
============================================
  Files           958      959       +1     
  Lines         40806    40855      +49     
  Branches       4632     4638       +6     
============================================
+ Hits          25971    26019      +48     
  Misses        13964    13964              
- Partials        871      872       +1     

Flag	Coverage Δ
sql-engine	63.68% <100.00%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mengweieric]

The change looks correct to me. Checking clusters.skipped() catches the case where a skipped cluster leaves the shard counts looking complete, and local or fully healthy cross-cluster searches are unaffected. The doc comment on ShardStats is the one change I would ask for before merge. The two test comments are optional.

The red build-linux (25, integration) check is unrelated to this change. It is the known locale flake in CalciteFlattenCommandIT.testFlattenNullField, which #5793 fixes.

[mengweieric]

This now lists a fourth cause, but the PPL protocol doc still lists three. Could you update the PARTIAL_RESULT row and the detail description in protocol.md to cover skipped remote clusters and the new _clusters segment?

It would also help to note what controls this case. The doc's advice to set search.default_allow_partial_results to false does not make these searches fail. Setting cluster.remote.<alias>.skip_unavailable to false does.

[ahkcs]

Updated the PARTIAL_RESULT row and detail description, and noted that skip_unavailable: false is what makes these fail.

[mengweieric]

Could both settings updates move inside the try? If the second one fails, finally never runs and the dead seed stays configured for the rest of the class.

[ahkcs]

Moved both inside the try.

[mengweieric]

Could this check the warning count first, as missingShardAttachesWarningNamingTheCounts does with assertEquals(1, warnings.length())? Then an unexpected extra warning would fail the test.

[ahkcs]

Added the count check.