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

Re-visit adding a no-op enable_fields_emulation flag to 8.0 #82485

Closed
cbuescher opened this issue Jan 12, 2022 · 8 comments · Fixed by #82539
Closed

Re-visit adding a no-op enable_fields_emulation flag to 8.0 #82485

cbuescher opened this issue Jan 12, 2022 · 8 comments · Fixed by #82539
Labels
:Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team team-discuss v8.0.0 v8.1.0

Comments

@cbuescher
Copy link
Member

cbuescher commented Jan 12, 2022

In #77749 (earlier PR #75745) we introduced a flag that allows to emulates the "fields" option on CCS search requests going to pre-7.10 nodes where this option doesn't exist. With this flag set, we are trying to do a best-effort attempt at emulating this option
in mixed cluster or CCS scenarios. At the time, the flag was added only in version 7.16 and 7.17. The reasoning behind this was that since CCS from 8.0 is only supported to 7.17 (N-1) and that version fully supports the “fields” option, we would not need that flag on 8.0.
I'm opening this issue since we might want to consider clients (e.g. Kibana) on 7.17 using this flag to be able to continue doing so in a rolling upgrade scenario, e.g. in a mixed ES 7.17/8.0 cluster. Currently, since we don't have the enable_fields_emulation flag on verison 8+, requests from a 7.17 client using it would fail because of an unknown rest parameter.
We would need to at least allow the flag to be set (a no-op behaviour would be sufficient I think because 7.17 doesn't need "fields" emulation) and can later remove it.

@cbuescher cbuescher added :Search/Search Search-related issues that do not fall into other categories team-discuss v8.0.0 v8.1.0 labels Jan 12, 2022
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Jan 12, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

@cbuescher
Copy link
Member Author

I briefly talked to @stacey-gammon how this flag is e.g. currently used on the Kibana side. There might be no usage so far at all, in which case I don't think we should add an empty no-op flag to 8.0. However, if there should be some usage on their side at any point in the future we might need to add the flag on the 8+ releases depending on how expectations are what happens when a 7.last client using this flag talks to a 8+ node in a rolling upgrade.

@stacey-gammon
Copy link
Contributor

Thanks @cbuescher for raising this. There may have been a miscommunication. It was my impression that the work being done on the Elasticsearch side, to implement the fields option compatibility layer, would Just Work in Kibana. I didn't realize we needed to set a flag to get it to work.

Because Kibana isn't using this flag, we might still have silent data loss in Discover under these circumstances. The good news is that I don't think we ever told customers it was fixed. Starting in 7.13 we documented it as a breaking change here (granted, it's unlikely users upgrading, especially to a version > 7.13, will read that note) and we have this open issue about it.

I guess the question now is, should Kibana set this flag in 7.17?

@kertal or @LeeDr - can one of you verify whether silent data loss is occurring in Discover under those circumstances? (can use a Kibana 7.17 communicating with ES 7.9 via CCS).

cc @giladgal @thomasneirynck

@LeeDr
Copy link

LeeDr commented Jan 12, 2022

I can work on a test.

@LeeDr
Copy link

LeeDr commented Jan 12, 2022

I currently have Kibana/Elasticsearch 7.17.0 with CCS to Elasticsearch 7.9.3.
What I'm seeing isn't ideal, but I don't think I'd call it "data loss". Discover does show all the docs. It just doesn't show all the fields from the 7.9.3 docs. But if I query on a field which isn't shown in the 7.9.3 docs I still get all the hits.

In my case I wrote docs to the remove 7.9.3 cluster with makelogs utility and let CCR replicate them to the 7.17.0 cluster. I have index patterns for each cluster individually and combined so I can verify a query response: 200 returns the same count of docs for each and 2x for the combined index pattern.

image

And if I change this advanced setting;

Read fields from _source
When enabled will load documents directly from `_source`. This is soon going to be deprecated. When disabled, will retrieve fields via the new Fields API in the high-level search service.
Default: false
discover:searchFieldsFromSource

image

With that change Discover looks completely normal to me;
image

@stacey-gammon
Copy link
Contributor

It just doesn't show all the fields from the 7.9.3 docs.

Yea, this is the data loss problem. Apologies, I first thought it was missing documents, but looking at past issues, it shows up as missing field data. The following two SDHs show this problem being reported in the field, and your test results indicate this is still a problem in 7.17.

@cbuescher
Copy link
Member Author

What I'm seeing isn't ideal, but I don't think I'd call it "data loss". Discover does show all the docs. It just doesn't show all the fields from the 7.9.3 docs. But if I query on a field which isn't shown in the 7.9.3 docs I still get all the hits.

Thanks for not calling this "data loss" since this evoces wrong reactions on casual reading. Its really that older pre-7.10 don't understand the "fields" option and don't report back the fields. The flag that we added in 7.16 tries to mitigate that by automatically using the "old" source filtering strategy for that and including the results in the "fields" response, so Kibana can simply consume it as if the were requested via "fields".
So the idea was that Kibana simply turns on the enable_fields_emulation flag when performing these discover searches. That should work, at least according to our own testing.

@cbuescher
Copy link
Member Author

I opened a PR for adding the flag to 8.1 and 8.0 so using it in 7.17 doesn't break in a rolling upgrade scenario.

cbuescher pushed a commit that referenced this issue Jan 13, 2022
This change adds the 'enable_fields_emulation' flag as a REST parameter to 8.0+ nodes
in order for clients on 7.last (e.g. Kibana) to be able to use it in rolling upgrade scenarios.
We don't need any implementation of the functionality behind it on 8.0+ nodes, because
CCS is only supported for them to at most 7.last which already understands and
implements the "fields" option, so emulation on older version is not necessary.

Closes #82485
cbuescher pushed a commit to cbuescher/elasticsearch that referenced this issue Jan 18, 2022
…#82539)

This change adds the 'enable_fields_emulation' flag as a REST parameter to 8.0+ nodes
in order for clients on 7.last (e.g. Kibana) to be able to use it in rolling upgrade scenarios.
We don't need any implementation of the functionality behind it on 8.0+ nodes, because
CCS is only supported for them to at most 7.last which already understands and
implements the "fields" option, so emulation on older version is not necessary.

Closes elastic#82485
cbuescher pushed a commit that referenced this issue Jan 18, 2022
…#82744)

This change adds the 'enable_fields_emulation' flag as a REST parameter to 8.0+ nodes
in order for clients on 7.last (e.g. Kibana) to be able to use it in rolling upgrade scenarios.
We don't need any implementation of the functionality behind it on 8.0+ nodes, because
CCS is only supported for them to at most 7.last which already understands and
implements the "fields" option, so emulation on older version is not necessary.

Closes #82485
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team team-discuss v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants