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

Emulate "fields" option on older versions #75745

Merged
merged 38 commits into from
Sep 14, 2021

Conversation

cbuescher
Copy link
Member

@cbuescher cbuescher commented Jul 27, 2021

We introduced the new "fields" option in search with version 7.10. With this
change we are trying to do a best-effort attempt at emulating this new behaviour
in mixed cluster or CCS scenarios where search requests using the "fields"
option also target older nodes or clusters. In that case, currently we don't
return anything in the "fields" section of the response.

This change tries to emulate the fields behaviour by modifying the request to
include the respective "_source" fields and then parsing them back into the
"fields" section on return. This will not be fully equivalent to the post-7.10 "fields"
functionality but at least try to include whatever we find in "_source" in earlier
versions.

@cbuescher cbuescher added WIP :Search/Search Search-related issues that do not fall into other categories v7.15.0 labels Jul 27, 2021
@cbuescher cbuescher force-pushed the ccs-fields-bridge branch 3 times, most recently from fd0c79e to 417a657 Compare July 28, 2021 09:41
Christoph Büscher added 2 commits July 28, 2021 18:35
We introduced the new "fields" option in search with version 7.10. With this
change we are trying to do a best-effort attempt at emulating this new behaviour
in mixed cluster or CCS scenarios where search requests using the "fields"
option also target older nodes or clusters. In that case, currently we don't
return anything in the "fields" section of the response. This change tried to
emulate the fields behaviour by modifying the request to include the respective
"_source" fields and then parsing them back into the "fields" section on return.
This will not be fully equivalent to the post-7.10 "fields" functionality but at
least try to include whatever we find in "_source" in earlier versions.

Currently Draft only, needs more testing in CCS scenarios but I'm opening this
here already to get some test coverage on the modifications so far.
@cbuescher cbuescher force-pushed the ccs-fields-bridge branch from 9cf4afb to 813d011 Compare July 28, 2021 16:36
@cbuescher cbuescher force-pushed the ccs-fields-bridge branch from cc8fb8c to 83fdb97 Compare July 29, 2021 20:33
@cbuescher cbuescher force-pushed the ccs-fields-bridge branch from aa42c03 to a05f7b6 Compare July 30, 2021 11:40
@cbuescher cbuescher marked this pull request as ready for review July 30, 2021 16:22
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Jul 30, 2021
@elasticmachine
Copy link
Collaborator

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

@cbuescher cbuescher removed the WIP label Jul 30, 2021
@cbuescher
Copy link
Member Author

cbuescher commented Jul 30, 2021

I think this is ready for a first general view. Known caveats: currently for mixed clusters only the default query_then_fetch search type is supported. While dfs_query_then_fetch is probably possible to implement in a somewhat similar way, it seems a bit more complex and we talked about query_then_fetch being a common enough default to support.
Tests probably need extension too, but at least the basic mixed cluster scenario and a remote CCS cluster on an older version are covered.
Also missing error handling in the case that the original search request contains both "fields" option and a "source" parameter with includes/excludes. I think we should error in this case but left this open for discussion so far.

@cbuescher cbuescher changed the title WIP: Emulate "fields" on older versions Emulate "fields" option on older versions Jul 30, 2021
@cbuescher
Copy link
Member Author

One general open question here is if we should put this bwc emulation behaviour behind a flag. I, for example, would prefer something like an "enable_fields_emulation" option on the "fields" parameter itself that's off by default and would only exist on 7.x branches going forwards (we don't need this emulation behaviour on 8+). That way clients interested in this best-effort emulation would need to consciously opt in to it with all the known limitations (only fetching from source, no formatting, no dfs-query-then-fetch).

@cbuescher
Copy link
Member Author

I added another commit that adds an "enable_fields_emulation" request parameter that switches the bwc behaviour off by default but can be enabled explicitely. We can always revert that change if we decide we don't want to do that.

@cbuescher
Copy link
Member Author

@jimczi thanks for the first round of reviews, I pushed some small changes and left questions regarding you other comments.

@cbuescher
Copy link
Member Author

Just to double check, when "_source" is disabled in a remote target index mapping, we will get an exception. This is the same as for non-CCS searches with source filtering or if the "fields" option is used on indices with disabled source, so I think we don't need any extra treatment here.

@cbuescher
Copy link
Member Author

@jimczi while adressing your review comments I found several ways to imporve and simplify the adapter class. In previous revisions the adapter and the internal field fetcher were created too often. Now the adapter only holds the basic information from the original request (like the fact that we need fields emulation at all and references that are needed to create the field fetcher later), and only creates more expensive resources like the field fetcher only once and when needed.
I didn't remove the second "minimize_roundtrip" path just yet, the simplifications make this aditional path look much less complex so it might be worth keeping it for completeness.

@cbuescher
Copy link
Member Author

I didn't remove the second "minimize_roundtrip" path just yet, the simplifications make this aditional path look much less complex so it might be worth keeping it for completeness.

I pushed a commit now that disables "minimize_roundtrip" for all requests that have the "fields emulation" enablement flag set and a non-empty fetch-fields parameter. This means that a potential "minimize_roundtrip=true" setting will be disregarded in that case. I don't know if that is a restriction we can live with and if that was the intention of the previous comment here.

Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

The change looks good to me. Thanks @cbuescher !

@jimczi
Copy link
Contributor

jimczi commented Sep 13, 2021

I pushed a commit now that disables "minimize_roundtrip" for all requests that have the "fields emulation" enablement flag set and a non-empty fetch-fields parameter.

The intent is to use this option in async_search where minimize roundtrip is not implemented so that's okish ;).

@cbuescher cbuescher merged commit 5a83dcd into elastic:7.x Sep 14, 2021
@cbuescher
Copy link
Member Author

To summarize the usage of the emulation layer:

  • clients need to set the enable_fields_emulation flag on the search request
  • the emulation behaviour will be limited to searches using the default query_then_fetch search type
  • if both the “fields” option and source filtering are mixed in the same search request, we will throw an error
  • 'minimize_roundtrip' settings is ignored when the emulation is effective

cbuescher pushed a commit that referenced this pull request Sep 14, 2021
@cbuescher
Copy link
Member Author

cbuescher commented Sep 14, 2021

Reverted since the serialization logic needs adapting, otherwise we break bwc for tranport on master

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:Clients Meta label for clients team Team:Search Meta label for search team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants