Skip to content

[Search] Fix isErrorResponse when cluster details are provided #166667

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lukasolson merged 1 commit into from
Sep 21, 2023
Merged

[Search] Fix isErrorResponse when cluster details are provided #166667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 120 additions & 0 deletions src/plugins/data/common/search/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,119 @@ describe('utils', () => {
expect(isError).toBe(true);
});

it('returns `false` if the response is not running and partial and contains failure details', () => {
const isError = isErrorResponse({
isPartial: true,
isRunning: false,
rawResponse: {
took: 7,
timed_out: false,
_shards: {
total: 2,
successful: 1,
skipped: 0,
failed: 1,
failures: [
{
shard: 0,
index: 'remote:tmp-00002',
node: '9SNgMgppT2-6UHJNXwio3g',
reason: {
type: 'script_exception',
reason: 'runtime error',
script_stack: [
'org.elasticsearch.server@8.10.0/org.elasticsearch.search.lookup.LeafDocLookup.getFactoryForDoc(LeafDocLookup.java:148)',
'org.elasticsearch.server@8.10.0/org.elasticsearch.search.lookup.LeafDocLookup.get(LeafDocLookup.java:191)',
'org.elasticsearch.server@8.10.0/org.elasticsearch.search.lookup.LeafDocLookup.get(LeafDocLookup.java:32)',
"doc['bar'].value < 10",
' ^---- HERE',
],
script: "doc['bar'].value < 10",
lang: 'painless',
position: {
offset: 4,
start: 0,
end: 21,
},
caused_by: {
type: 'illegal_argument_exception',
reason: 'No field found for [bar] in mapping',
},
},
},
],
},
_clusters: {
total: 1,
successful: 1,
skipped: 0,
details: {
remote: {
status: 'partial',
indices: 'tmp-*',
took: 3,
timed_out: false,
_shards: {
total: 2,
successful: 1,
skipped: 0,
failed: 1,
},
failures: [
{
shard: 0,
index: 'remote:tmp-00002',
node: '9SNgMgppT2-6UHJNXwio3g',
reason: {
type: 'script_exception',
reason: 'runtime error',
script_stack: [
'org.elasticsearch.server@8.10.0/org.elasticsearch.search.lookup.LeafDocLookup.getFactoryForDoc(LeafDocLookup.java:148)',
'org.elasticsearch.server@8.10.0/org.elasticsearch.search.lookup.LeafDocLookup.get(LeafDocLookup.java:191)',
'org.elasticsearch.server@8.10.0/org.elasticsearch.search.lookup.LeafDocLookup.get(LeafDocLookup.java:32)',
"doc['bar'].value < 10",
' ^---- HERE',
],
script: "doc['bar'].value < 10",
lang: 'painless',
position: {
offset: 4,
start: 0,
end: 21,
},
caused_by: {
type: 'illegal_argument_exception',
reason: 'No field found for [bar] in mapping',
},
},
},
],
},
},
},
hits: {
total: {
value: 1,
relation: 'eq',
},
max_score: 0,
hits: [
{
_index: 'remote:tmp-00001',
_id: 'd8JNlYoBFqAcOBVnvdqx',
_score: 0,
_source: {
foo: 'bar',
bar: 1,
},
},
],
},
},
});
expect(isError).toBe(false);
});

it('returns `false` if the response is running and partial', () => {
const isError = isErrorResponse({
isPartial: true,
Expand Down Expand Up @@ -66,6 +179,13 @@ describe('utils', () => {
});
expect(isError).toBe(true);
});

it('returns `true` if the response does not indicate isRunning', () => {
const isError = isCompleteResponse({
rawResponse: {},
});
expect(isError).toBe(true);
});
});

describe('isPartialResponse', () => {
Expand Down
23 changes: 21 additions & 2 deletions src/plugins/data/common/search/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,36 @@ import { AggTypesDependencies } from '..';
import type { IKibanaSearchResponse } from './types';

/**
* From https://github.com/elastic/elasticsearch/issues/55572: "When is_running is false, the query has stopped, which
* may happen due to ... the search failed, in which case is_partial is set to true to indicate that any results that
* may be included in the search response come only from a subset of the shards that the query should have hit."
* @returns true if response had an error while executing in ES
*/
export const isErrorResponse = (response?: IKibanaSearchResponse) => {
return !response || !response.rawResponse || (!response.isRunning && !!response.isPartial);
return (
!response ||
!response.rawResponse ||
(!response.isRunning &&
!!response.isPartial &&
// See https://github.com/elastic/elasticsearch/pull/97731. For CCS with ccs_minimize_roundtrips=true, isPartial
// is true if the search is complete but there are shard failures. In that case, the _clusters.details section
// will have information about those failures. This will also likely be the behavior of CCS with
// ccs_minimize_roundtrips=false and non-CCS after https://github.com/elastic/elasticsearch/issues/98913 is
// resolved.
!response.rawResponse?._clusters?.details)

@piergm piergm Sep 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left the same comment here, sorry for the duplicate comment.

I am not sure that checking _clusters.details is enough. I think we must check _clusters.details.clusterAlias.failures
The new response for CCS is now including _clusters.details both for error and non error responses.
An example of non error details section is:

"details": {
  "(local)": {
    "status": "successful",
    "indices": foo",
    "took": 123,
    "timed_out": false,
    "_shards": {
      ...
    }
  },
  "remote": {
    "status": "successful",
    "indices": "foo",
    "took": 234,
    "timed_out": false,
    "_shards": {
      ...
    }
  }
}

And an example of error details section:

"details": {
  "remote": {
      "status": "partial",
      "indices": "foo",
      "took": 123,
      "timed_out": false,
      "_shards": {
        "total": 6,
        "successful": 5,
        "skipped": 0,
        "failed": 1  
      },
      "failures": [ 
        {
          "shard": 1,
          "index": "remote:foo",
          "node": ...,
          "reason": {
            "type": "query_shard_exception",
            "reason": "failed to create query: exception message here",
            ...
          }
        }
      ]
  }
}

@lukasolson lukasolson Sep 19, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for weighing in here @piergm.

To give some context, this check (isErrorResponse) will soon be removed entirely (see #164506). Right now, if isErrorResponse returns true, then we throw an error from the Kibana server and clients will receive a 500 error. This behavior is not ideal, but it has been this way for years now and we were hoping to limit the scope as much as possible to speed up a potential patch release.

The change in this PR is less about identifying when a response from ES is/is not an actual error, but instead opening an escape hatch to not throw an error if we encounter the new response format. In other words, the check !response.rawResponse?._clusters?.details is not actually for identifying whether or not the response contains errors, but rather for identifying that the response format is the new format.

@piergm piergm Sep 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for clarifying this @lukasolson. Than this LGTM!

);
};

/**
* @returns true if response is completed successfully
*/
export const isCompleteResponse = (response?: IKibanaSearchResponse) => {
return Boolean(response && !response.isRunning && !response.isPartial);
// Some custom search strategies do not indicate whether they are still running. In this case, assume it is complete.
if (response && !response.hasOwnProperty('isRunning')) {
return true;
}

return !isErrorResponse(response) && Boolean(response && !response.isRunning);
};

/**
Expand Down