EQL: Minimise CCS roundtrips #76076

bpintea · 2021-08-04T11:06:24Z

This introduces an optimisation of the EQL requests when these target
one remote cluster only (i.e. no mixed local and remote indices or
multiple remote clusters). In this case, the EQL request is forwarded
to the remote cluster and executed there, instead of having the local
cluster perform multiple queries of the remote cluster.

This behaviour is enabled by default, a new request parameter
ccs_minimize_roundtrips can be set to false to disable it.

This intorduces an optimisation of the EQL requests when these target one remote cluster only (i.e. no mixed local and remote indices or multiple remote clusters). In this case, the EQL request is forwarded to the remote cluster and executed there, instead of having the local cluster perform multiple querries of the remote cluster.

jrodewig

Docs LGTM. I left some non-blocking nits.

jrodewig · 2021-08-04T12:35:58Z

docs/reference/eql/eql-search-api.asciidoc

+(Optional, Boolean)
+
+If `true`, network round-trips between the local and the remote cluster are
+minimized when executing cross-cluster search (CCS) requests.


Just fixes some wrapping.

Suggested change

(Optional, Boolean)

+

If `true`, network round-trips between the local and the remote cluster are

minimized when executing cross-cluster search (CCS) requests.

(Optional, Boolean) If `true`, network round-trips between the local and the

remote cluster are minimized when running cross-cluster search (CCS) requests.

Thanks, fixed!

jrodewig · 2021-08-04T12:41:34Z

docs/reference/eql/eql-search-api.asciidoc

+If `true`, network round-trips between the local and the remote cluster are
+minimized when executing cross-cluster search (CCS) requests.
+
+This setting is currently only effective if the request does not include both


I'd use option or parameter to avoid confusion with cluster/index settings.

Suggested change

This setting is currently only effective if the request does not include both

This option is currently only effective if the request does not include both

Adress doc review comments. Update error messages

elasticmachine · 2021-08-04T13:46:21Z

Pinging @elastic/es-ql (Team:QL)

Luegg

Only some minor comments and questions, otherwise all makes sense to me 👍

Luegg · 2021-08-04T13:07:54Z

docs/reference/eql/eql-search-api.asciidoc

@@ -89,6 +89,15 @@ request that targets only `bar*` still returns an error.
 +
 Defaults to `true`.

+`ccs_minimize_roundtrips`::


Can you give some background behind the decision to make this configurable? Is there a use case for not minimizing round trips or is that a general convention for configurability of optimizations like this?

We may want to link to https://www.elastic.co/guide/en/elasticsearch/reference/master/modules-cross-cluster-search.html#ccs-network-delays.

👍 thanks a lot @jrodewig , I wasn't aware of this doc yet. That clarifies a lot!

Right, the setting follows suite of the one for search and is similar to it, but I've hesitated to add the reference, since we don't actually link the two. We could add an explanation note to clarify the distinction, though.

Luegg · 2021-08-04T14:22:12Z

...ck/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/plugin/TransportEqlSearchAction.java

+        if (clusterAliases.size() != 1 || clusterAliases.contains(RemoteClusterAware.LOCAL_CLUSTER_GROUP_KEY)) {
+            return false;
+        }
+        // The remote async ID would not be valid on local cluster; furthermore on results fetching we would know neither if the ID is


Maybe it's worth to mention this limitation in the docs as well?

It might, but wanted to have the TODO proposal reviewed, in case it emerges as a "blocker" for this optimisation.

Luegg · 2021-08-04T14:31:31Z

...ck/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/plugin/TransportEqlSearchAction.java

+            transportService.sendRequest(transportService.getRemoteClusterService().getConnection(clusterAlias),
+                EqlSearchAction.INSTANCE.name(), request.indices(remoteIndices), TransportRequestOptions.EMPTY,
+                new ActionListenerResponseHandler<>(wrap(r -> listener.onResponse(qualifyHits(r, clusterAlias)),
+                    e -> listener.onFailure(qualifyException(e, request.indices(), clusterAlias))),


Suggested change

e -> listener.onFailure(qualifyException(e, request.indices(), clusterAlias))),

e -> listener.onFailure(qualifyException(e, remoteIndices, clusterAlias))),

Minor change but at least to me it would help to be explicit about this because the mutation of request.indices() is very subtle. Assuming that it's indeed the remote indices that should be passed.

No, it's a good point. I think current format was more of a copy that happened to work. Will update with next batch of changes.
And yes, we need the remote-cluster-name: prefix-stripped remote indices passed.

costin

Left a first round of comments.
One thing that missing from this PR is the client EQL code which needs to be updated (in a backwards compatible way) with the new settings.

costin · 2021-08-05T07:57:49Z

docs/reference/eql/eql-search-api.asciidoc

+(Optional, Boolean) If `true`, network round-trips between the local and the
+remote cluster are minimized when running cross-cluster search (CCS) requests.
+
+This option is currently only effective if the request does not include both


This needs rephrasing:

This option is effective for requests that target data fully contained in one remote cluster; when data is spread across multiple clusters, the setting is ignored.

This explanation is compacter, though maybe less explicit. I've applied the proposal, though.

costin · 2021-08-05T07:58:55Z

...lti-cluster-with-security/src/test/java/org/elasticsearch/xpack/eql/EqlRestValidationIT.java

@@ -17,21 +17,19 @@

    @Override
    protected String getInexistentIndexErrorMessage() {
-        return "\"caused_by\":{\"type\":\"verification_exception\",\"reason\":\"Found 1 problem\\nline -1:-1: Unknown index ";
+        return "\"root_cause\":[{\"type\":\"index_not_found_exception\",\"reason\":\"no such index ";


Subtle different - what's the reason behind it?

"Qualifying" the index in the exception replaces the VerificationException with an IndexNotFoundException, to "harmonise" the failure reporting. This would be needed to not differentiate the failure format depending on which request parameters are set.

costin · 2021-08-05T08:05:35Z

...-cluster-with-security/src/test/java/org/elasticsearch/xpack/eql/RemoteClusterTestUtils.java

@@ -15,11 +17,10 @@ public static String remoteClusterIndex(String indexName) {
    }

    public static String remoteClusterPattern(String pattern) {
-        StringBuilder sb = new StringBuilder();
+        StringJoiner sj = new StringJoiner(",");
        for (String index: pattern.split(",")) {


For future reference String.split is expensive since it compiles a regex pattern - however it does have a shortcut (at least in the recent JVMs) where for single chars it does string iteration. For the former case better to use Strings.commaDelimitedListToStringArray

costin · 2021-08-05T08:09:00Z

...ck/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/plugin/TransportEqlSearchAction.java

+            return false;
+        }
+        // The remote async ID would not be valid on local cluster; furthermore on results fetching we would know neither if the ID is
+        // remote or not, nor which remote cluster it belongs to (TODO: could rewrite the ID to smth like [alias:ID])


I'm okay with dealing with the async bit after this gets merged. Curious how async search and CCS deals with this scenario.

"Cross-cluster search is supported only with ccs_minimize_roundtrips set to false.". I.e. it won't optimise it either, it'll query the remote cluster, but store the results locally.

costin · 2021-08-05T08:12:16Z

...ck/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/plugin/TransportEqlSearchAction.java

+    private static EqlSearchResponse qualifyHits(EqlSearchResponse r, String clusterAlias) {
+        EqlSearchResponse.Hits hits = r.hits();
+        List<EqlSearchResponse.Sequence> sequences = qualifySequences(hits.sequences(), clusterAlias);
+        List<EqlSearchResponse.Event> events = sequences != null ? null : qualifyEvents(hits.events(), clusterAlias, true);
+        return new EqlSearchResponse(new EqlSearchResponse.Hits(events, sequences, hits.totalHits()), r.took(), r.isTimeout());
+    }
+
+    private static List<EqlSearchResponse.Sequence> qualifySequences(List<EqlSearchResponse.Sequence> sequences, String clusterAlias) {
+        List<EqlSearchResponse.Sequence> qualifiedSequences = null;
+        if (sequences != null && sequences.isEmpty() == false) {
+            qualifiedSequences = new ArrayList<>(sequences.size());
+            for (EqlSearchResponse.Sequence s : sequences) {
+                qualifiedSequences.add(new EqlSearchResponse.Sequence(s.joinKeys(), qualifyEvents(s.events(), clusterAlias, false)));
+            }
+        }
+        return qualifiedSequences;
+    }
+
+    private static List<EqlSearchResponse.Event> qualifyEvents(List<EqlSearchResponse.Event> events, String clusterAlias,
+                                                               boolean emptyListIfNoEvents) {
+        List<EqlSearchResponse.Event> qualifiedEvents = null;
+        if (events != null && events.isEmpty() == false) {
+            qualifiedEvents = new ArrayList<>(events.size());
+            for (EqlSearchResponse.Event e : events) {
+                qualifiedEvents.add(new EqlSearchResponse.Event(buildRemoteIndexName(clusterAlias, e.index()), e.id(), e.source(),
+                    e.fetchFields()));
+            }
+        }
+        return qualifiedEvents == null && emptyListIfNoEvents ? new ArrayList<>() : qualifiedEvents;
+    }


Unless I'm missing something, these methods copy the response to prefix the index which is a waste.
Either change the actual returned objects to be mutable so that the remote cluster alias is added to the index or push the clusterAlias info so it's available during the EqlSearchResponse.* creation.

I've change the mutability to allow setting an index name. The alternative would require passing the cluster name thorough multiple layer, which is doable, but seemed a bit more contrived.

costin · 2021-08-05T08:13:50Z

...ck/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/plugin/TransportEqlSearchAction.java

+                // index [...]"); this is also similar to a non-CCS `POST inexistent/_eql/search?ignore_unavailable=false`, but
+                // unfortunately unlike an inexistent pattern search: `POST inexistent*/_eql/search?ignore_unavailable=false, which raises a
+                // VerificationException as it's root cause. I.e. the failures are not homogenous.
+                finalException = new RemoteTransportException(e.getMessage(), new IndexNotFoundException(qualifiedIndex));


When @astefan come back please raise this with him since he worked on something similar. It's worth creating an issue to see whether the two errors can be aligned and if not why?

Great. It would seem good to me to return/report a not found index in the same way; i.e. no matter if the verifier finds the query error or other layer under EQL. (This could probably also be a subsequent PR.)

costin · 2021-08-05T08:15:04Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/util/RemoteClusterRegistry.java

-            Strings.splitStringByCommaToArray(indexPattern));
-        indicesMap.remove(RemoteClusterAware.LOCAL_CLUSTER_GROUP_KEY);
-        return indicesMap;
+    public Set<String> clusterAliases(String[] indices, boolean discardLocal) {


Does discardLocal has any other value than true? If not, better to keep the method specialized.

Does discardLocal has any other value than true?

Yes, the optimisation code needs to know if the queried indices are all local or not (so it calls with this parameter set to false).

costin · 2021-08-05T08:18:48Z

...ck/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/plugin/TransportEqlSearchAction.java

-        EqlConfiguration cfg = new EqlConfiguration(request.indices(), zoneId, username, clusterName, filter,
+        Set<String> clusterAliases = remoteClusterRegistry.clusterAliases(request.indices(), false);
+        if (canMinimizeRountrips(request, clusterAliases)) {
+            String clusterAlias = clusterAliases.iterator().next();


Instead of using two methods - canMinimize plus calling the cluster, how about combining them into a method that returns the cluster on which to re-route the call or otherwise returning null.

String singleCssCluster = minimizedCss(request, clusterAliases); if (singleCssCluster != null) { .... } else { ... }

This helps with the different methods around checking, selecting and massaging the string into one method.

I did initially consider this format too (before the small RemoteClusterRegistry refactoring), but found delegating the decision to a method a clearer implementation (similar to TransportSearchAction#shouldMinimizeRoundtrips().

costin · 2021-08-05T08:21:01Z

...ck/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/plugin/TransportEqlSearchAction.java

+            String clusterAlias = clusterAliases.iterator().next();
+            String[] remoteIndices = new String[request.indices().length];
+            for (int i = 0; i < request.indices().length; i++) {
+                remoteIndices[i] = request.indices()[i].substring(clusterAlias.length() + /*`:`*/1);


That inline comment is too l33t. please move it after the statement or above it since as it stands requires multi-parsing.

Pulled that into a trailing comment.

- rephrase docs; - EqlSearchResponse.Event allows setting the index value; - update comment. Also, fix a case of index qualifying in an exception / failure case.

bpintea · 2021-08-08T20:40:32Z

One thing that missing from this PR is the client EQL code which needs to be updated (in a backwards compatible way) with the new settings.

EqlClient? I'll look into it.

costin · 2021-08-09T07:48:50Z

One thing that missing from this PR is the client EQL code which needs to be updated (in a backwards compatible way) with the new settings.

EqlClient? I'll look into it.

Since it's a parameter, it's the request that needs to be changed not the client - e.g. EqlSearchRequest

Add de-/serialization of ccs_minimize_roundtrips setting to action/EqlSearchRequest.

bpintea · 2021-08-18T08:58:26Z

One thing that missing from this PR is the client EQL code which needs to be updated (in a backwards compatible way) with the new settings.

Serialisation of the new parameter has been added.

astefan

LGTM

elasticsearchmachine · 2021-08-18T12:03:27Z

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

To backport manually run backport --upstream elastic/elasticsearch --pr 76076

This introduces an optimisation of the EQL requests when these target one remote cluster only (i.e. no mixed local and remote indices or multiple remote clusters). In this case, the EQL request is forwarded to the remote cluster and executed there, instead of having the local cluster perform multiple queries to the remote cluster. (cherry picked from commit 7a5ac3e)

bpintea added >enhancement v8.0.0 :Analytics/EQL EQL querying v7.15.0 labels Aug 4, 2021

bpintea requested review from costin, astefan, Luegg and jrodewig August 4, 2021 11:06

jrodewig approved these changes Aug 4, 2021

View reviewed changes

Adress doc review comments. Update error messages

889d8e6

Adress doc review comments. Update error messages

bpintea marked this pull request as ready for review August 4, 2021 13:46

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Aug 4, 2021

Luegg approved these changes Aug 4, 2021

View reviewed changes

costin requested changes Aug 5, 2021

View reviewed changes

bpintea added 2 commits August 8, 2021 22:00

Adress review comments.

e754374

- rephrase docs; - EqlSearchResponse.Event allows setting the index value; - update comment. Also, fix a case of index qualifying in an exception / failure case.

Merge branch 'master' into enh/optimize_ccs

8917176

bpintea added 3 commits August 17, 2021 19:09

Merge branch 'master' into enh/optimize_ccs

a79c843

Address review comments

500b08c

Add de-/serialization of ccs_minimize_roundtrips setting to action/EqlSearchRequest.

Merge branch 'master' into enh/optimize_ccs

a41c713

astefan approved these changes Aug 18, 2021

View reviewed changes

bpintea added the auto-backport Automatically create backport pull requests when merged label Aug 18, 2021

bpintea merged commit 7a5ac3e into elastic:master Aug 18, 2021

bpintea deleted the enh/optimize_ccs branch August 18, 2021 12:01

bpintea added the backport pending label Aug 18, 2021

bpintea removed the backport pending label Aug 18, 2021

bpintea mentioned this pull request Aug 30, 2021

EQL: investigate CCS optimisations #74960

Closed

jakelandis added v8.0.0-alpha2 and removed v8.0.0 labels Sep 15, 2021

	This setting is currently only effective if the request does not include both
	This option is currently only effective if the request does not include both

	e -> listener.onFailure(qualifyException(e, request.indices(), clusterAlias))),
	e -> listener.onFailure(qualifyException(e, remoteIndices, clusterAlias))),

EQL: Minimise CCS roundtrips #76076

EQL: Minimise CCS roundtrips #76076

Conversation

bpintea commented Aug 4, 2021

jrodewig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 4, 2021

Luegg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea commented Aug 8, 2021

costin commented Aug 9, 2021

bpintea commented Aug 18, 2021

astefan left a comment

Choose a reason for hiding this comment

elasticsearchmachine commented Aug 18, 2021

💔 Backport failed