Skip to content
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.

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

[data.search] Send ccs_minimize_roundtrips=true for async search requests #159848

Merged
merged 6 commits into from Jun 20, 2023

Conversation

lukasolson
Copy link
Member

@lukasolson lukasolson commented Jun 15, 2023

Summary

Resolves #159706. Sends ccs_minimize_roundtrips=true for async search requests to decrease network delays when cross-cluster search is at play.

Checklist

For maintainers

@lukasolson lukasolson added Feature:Search Querying infrastructure in Kibana release_note:fix Project:AsyncSearch Background search, partial results, async search services. Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) v8.9.0 labels Jun 15, 2023
@lukasolson lukasolson self-assigned this Jun 15, 2023
@lukasolson lukasolson requested review from a team as code owners June 15, 2023 21:37
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@lukasolson lukasolson added the ci:cloud-deploy Create or update a Cloud deployment label Jun 15, 2023
@@ -50,6 +51,8 @@ export async function getDefaultAsyncSubmitParams(
return {
// TODO: adjust for partial results
batched_reduce_size: searchConfig.asyncSearch.batchedReduceSize,
// Decreases delays due to network when using CCS
ccs_minimize_roundtrips: false,
Copy link
Member

Choose a reason for hiding this comment

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

QQ: if I understand correctly it should be set to true to decrease delays?

Copy link

Choose a reason for hiding this comment

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

That's correct. I believe Kibana wants to set it to true from now on: #159706

The change on the ES side (elastic/elasticsearch#96012) now supports minimizing round trips with async_search, but still leaves ccs_minimize_rountrips=false as the default.

@lukasolson lukasolson changed the title [data.search] Send ccs_minimize_roundtrips=false for async search requests [data.search] Send ccs_minimize_roundtrips=true for async search requests Jun 20, 2023
@lukasolson lukasolson requested review from kertal and quux00 June 20, 2023 14:20
@kibana-ci
Copy link
Collaborator

kibana-ci commented Jun 20, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #59 / Cloud Security Posture Findings Page Table Sort sorts by a column, should be case sensitive/insensitive depending on the column

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 13 15 +2
securitySolution 411 415 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 14 16 +2
securitySolution 494 498 +4
total +6

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @lukasolson

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Tested in Discover in a cloud deployment, and everything seems to be working as expected. LGTM 👍

@lukasolson lukasolson merged commit 69849ee into elastic:main Jun 20, 2023
24 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 20, 2023
@quux00
Copy link

quux00 commented Jun 21, 2023

@lukasolson One consideration here is that prior to merging elastic/elasticsearch#96012, Elasticsearch does not support ccs_minimize_roundtrips=true for the POST _async_search endpoint (it will return an error). For older versions you'll still need to send ccs_minimize_roundtrips=false (or don't send the parameter, since false is the default). Will that be handled correctly with this change?

Some more details on that

As long as the querying cluster coordinator you are sending the _async_search CCS query to supports the ccs_minimize_roundtrips=true flag, it will work, even if the remote clusters have an earlier version or other nodes within the same cluster are running an older version.

However, if Kibana sends ccs_minimize_roundtrips=true to a querying cluster coordinator that is running an older version of ES (8.8.1 or earlier), it will get back an immediate error [ccs_minimize_roundtrips] is not supported on async search queries.

So if customers can run a mixed version system where Kibana is on say 8.9.0 and the ES coordinator receiving the query is on 8.8.0, then the Kibana code would need to build in logic to check the version of the coordinator (maybe with GET /) or watch for that error message and change ccs_minimize_roundtrips back to false to do the query.

Here's the error you will get:

// status
HTTP/1.1 400 Bad Request

// response body
{
  "error": {
    "root_cause": [
      {
        "type": "action_request_validation_exception",
        "reason": "Validation Failed: 1: [ccs_minimize_roundtrips] is not supported on async search queries;"
      }
    ],
    "type": "action_request_validation_exception",
    "reason": "Validation Failed: 1: [ccs_minimize_roundtrips] is not supported on async search queries;"
  },
  "status": 400
}

@lukasolson
Copy link
Member Author

@quux00 Correct me if I'm wrong, but we don't have any way to know which remote CCS clusters we're hitting when sending a search. In the past, this sort of thing has been handled internally by ES, which correctly forwards (or doesn't forward) certain parameters to remote clusters running a previous version. Is this something that can be done?

@lukasolson
Copy link
Member Author

Here's an example of when ES implemented a parameter as a no-op for BWC: elastic/elasticsearch#82744

@quux00
Copy link

quux00 commented Jun 21, 2023

For this flag with CCS, Kibana only needs to be concerned with the version of the ES coordinator - meaning the ES node that Kibana is connecting to issue the search request. The parsing of the "ccs_minimize_roundtrips" flag (and then deciding whether to minimize roundtrips) is done on the coordinator node only. After that it is just a "normal" search on each shard and is fully backwards compatible with all of the nodes the coordinator sends the search request to (both nodes of the same cluster and remote clusters), even if they are of older versions.

I don't know how Kibana works in terms of selecting a coordinator (whether it always uses the same one or does a round robin among nodes of a cluster). If you always connect to the same coordinator, you could perhaps cache the ES version and decide whether it is safe to send ccs_minimize_roundtrips=true for subsequent searches. But if you randomly select coordinators, then you may have to try setting ccs_minimize_roundtrips=true first and then if you get the error I mentioned above, switch to false?

Here's an example of when ES implemented a parameter as a no-op for BWC: elastic/elasticsearch#82744

Reading over this one, this looks like the opposite scenario. In that case, a flag that had some action in older versions become a "no-op" in a newer version so the newer version of ES could just write some code to ignore it.

In the ccs_minimize_roundtrips case here we are adding new functionality that only exists in the new version and will not work in the older ones, so we can't add this type of handler in the new version.

@javanna
Copy link
Member

javanna commented Jun 21, 2023

Bottom line: there is no problem when it comes to CCS with supporting ccs_minimize_roundtrips if remote clusters are on older versions that did not support minimizing roundtrips in async search. It's a functionality that only the coordinator node needs to know about.

If the coordinating cluster is a mixed version cluster, we rely on the fact that Kibana is on the lowest version of all nodes, and that it is upgraded last. If this rule of thumb is followed, no special handling is necessary within Kibana. If Kibana on a version that's higher than some of the nodes in the coordinating cluster, all bets are off, but that is in general, not only for this specific change.

@fkanout
Copy link
Contributor

fkanout commented Jun 22, 2023

Hey @lukasolson, under Observability, the Alerts table can't load the alerts when ccs_minimize_roundtrips=true, and it throws this error
Screenshot 2023-06-22 at 17 23 12

Any suggestions to fix this? I'm using oblt-edge

@lukasolson
Copy link
Member Author

@fkanout What version of ES are you using?

@fkanout
Copy link
Contributor

fkanout commented Jun 22, 2023

I'm using v8.8.0

@lukasolson
Copy link
Member Author

I'm using v8.8.0

This change will only be in 8.9+, which will only be compatible with ES 8.9+.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:cloud-deploy Create or update a Cloud deployment Feature:Search Querying infrastructure in Kibana Project:AsyncSearch Background search, partial results, async search services. release_note:fix Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data.search] Minimize round trips by default
9 participants