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

Properly propagate the search request context & headers to sub-requests #10979

Closed
uboness opened this issue May 5, 2015 · 4 comments · Fixed by #11060
Closed

Properly propagate the search request context & headers to sub-requests #10979

uboness opened this issue May 5, 2015 · 4 comments · Fixed by #11060
Assignees
Labels
>bug :Search/Search Search-related issues that do not fall into other categories

Comments

@uboness
Copy link
Contributor

uboness commented May 5, 2015

Each transport request holds context and headers that represent the general context of the executed actions. There are actions that spawn other requests and we make sure that the context/headers of the original request is passed along to the sub-requests.

In the case of a search request, we have some cases where this context/headers passing doesn't happen and we need to fix that (we need to make sure the context & headers are not lost at any point in the execution). The reason for this today is that some spawned request simply don't have access to the original request. For example, TermsLookup used by the external terms filter executes a search during the parsing of the query. It has no access to the original search request that was executed and therefore cannot pass along the context & headers.

To solve that we can use the SearchContext and let it hold the search request context & headers (maybe we can have the SearchContext extends ContextHolder). During the execution of a search, any phase that is part of the execution has access to the search context using the thread local based SearchContext.current(). We need to make sure, to set the request context & headers on the SearchContext when it is created. From there on, we can extract these request context & headers everywhere we need to and pass it to the sub-request (a la TermsLookup).

Other sub request we need to look at:

  • MoreLikeThisQuery
  • GeoShapeFilter and GeoShapeQuery with a indexed shape
  • Indexed Scripts
  • Phrase Suggester with a collate query or filter
  • Search Templates with indexed templates
@uboness uboness added >bug :Search Templates :Search/Search Search-related issues that do not fall into other categories labels May 5, 2015
@clintongormley
Copy link

@spinscale please could you take a look when you have a moment

@spinscale
Copy link
Contributor

checked the source, my current idea of implementing it is

  • Make SearchContext implementing an interface that adheres to ContextHolder, propagate to ShardSearchRequest, maybe just changing DefaultSearchContext is enough, needs to be checked
  • ShardSearchRequest also needs to implement that new interface, one of the two subclasses, ShardSearchTransportRequest already does, one needs to be extended ShardSearchLocalRequest
  • Both classes have access to the original SearchRequest and thus can copy the context
  • Make sure all of the above requests can make use of this infra
  • Search for possible more requests that need this
  • Copy headers need to be copied as well

Tricky: Testing this

@uboness
Copy link
Contributor Author

uboness commented May 6, 2015

Later: Do headers need to be copied as well

while we're at it, I'd just copy the headers as well.. why not

@spinscale
Copy link
Contributor

Implementation note: Percolate-by-id must be supported as well

spinscale added a commit to spinscale/elasticsearch that referenced this issue May 8, 2015
Whenever a query parser (or any other component) issues another
request as part of a request, the headers and the context has to
be supplied as well.

In order to do this, the `SearchContext` has to have those headers
available, which in turn means, the shard level request needs to
copy those from the original `SearchRequest`

This commit introduces two new interface to supply the needed methods
to work with context and headers.

TODO

* Validate this approach
* PercolateContext is pretty dumb right now and just implements the methods
  without acting
* Testing: Context testing is tricky, havent found a smart way so far

Closes elastic#10979
spinscale added a commit to spinscale/elasticsearch that referenced this issue May 15, 2015
Whenever a query parser (or any other component) issues another
request as part of a request, the headers and the context has to
be supplied as well.

In order to do this, the `SearchContext` has to have those headers
available, which in turn means, the shard level request needs to
copy those from the original `SearchRequest`

This commit introduces two new interface to supply the needed methods
to work with context and headers.

TODO

* Validate this approach
* PercolateContext is pretty dumb right now and just implements the methods
  without acting
* Testing: Context testing is tricky, havent found a smart way so far

Closes elastic#10979
spinscale added a commit to spinscale/elasticsearch that referenced this issue May 18, 2015
Whenever a query parser (or any other component) issues another
request as part of a request, the headers and the context has to
be supplied as well.

In order to do this, the `SearchContext` has to have those headers
available, which in turn means, the shard level request needs to
copy those from the original `SearchRequest`

This commit introduces two new interface to supply the needed methods
to work with context and headers.

TODO

* Validate this approach
* PercolateContext is pretty dumb right now and just implements the methods
  without acting
* Testing: Context testing is tricky, havent found a smart way so far

Closes elastic#10979
spinscale added a commit to spinscale/elasticsearch that referenced this issue May 18, 2015
Whenever a query parser (or any other component) issues another
request as part of a request, the headers and the context has to
be supplied as well.

In order to do this, the `SearchContext` has to have those headers
available, which in turn means, the shard level request needs to
copy those from the original `SearchRequest`

This commit introduces two new interface to supply the needed methods
to work with context and headers.

Closes elastic#10979
spinscale added a commit that referenced this issue May 18, 2015
Whenever a query parser (or any other component) issues another
request as part of a request, the headers and the context has to
be supplied as well.

In order to do this, the `SearchContext` has to have those headers
available, which in turn means, the shard level request needs to
copy those from the original `SearchRequest`

This commit introduces two new interface to supply the needed methods
to work with context and headers.

Closes #10979
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Search Templates labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants