Skip to content

Refactor SearchRequest to be parsed on the coordinating node #13752

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

Closed

Conversation

colings86
Copy link
Contributor

This change refactors the SearchRequest so it is parsed on the coordinating node and then sent as a serialised object to the shards. The SearchSourceBuilder encompasses the state of the bpdy of the search request (know in the code as source) and is embedded in the SearchRequest. On the shard the SearchSourceBuilder is used to populate the SearchContext where we used to parse the source JSON.

A few things to note:

  • The Java API has been changed to only accept builders in the SearchRequest, SearchRequestBuilder, SearchSourceBuilder and other similar classes. This means that all methods that accepted BytesReference, byte[], String, BytesArray etc. in the search API (and derivatives) should have been removed
  • Some variables inside the SearchSourceBuilder are stored as BytesReference object as their builder objects have not yet been refactored. The SearchSourceBuilder converts the incoming Builder into the BytesReference to serialise to the shards and the parsing of this JSON is still handled in SearchService.parseSource(). Over time these will be refactored so these builders can also be stored as objects in SearchSourceBuilder and serialised to the shards properly.

@colings86
Copy link
Contributor Author

This has been replaced by #13859

dadoonet and others added 20 commits October 6, 2015 12:18
Related to #13602
(cherry picked from commit ff4cdab)
(cherry picked from commit 8b46a4b)
After `field` has been renamed to `fields` in #13902
The field is optional everywhere else but in the serialization methods, which causes problems. Also expanded tests so that they can catch this type of problem.

Closes #13963
Make strategy optional in GeoShapeQueryBuilder readFrom and writeTo
This methods was only used in tests and can be replaced by calling
`ScriptEngineService.executable(compiledScript, vars).run()` instead.
…cute

Remove ScriptEngineService.execute.
Plugin cli tools configures logging with whatever is in the logging.yml.
If a file appender is configured for any of the logs this will cause creation
of an empty log file. If a plugin was for example installed as root it will
create empty logs at es.home/logs.
This is problematic when for example plugins are installed as root and es is run
as service. Logs will then be created in /usr/share/elasticsearch/logs
and can later not be removed by for example dpkg -r or -purge.

To avoid this, configure the logger to use an appender that writes to the same
output that plugin cli tool does. This allows other components that are called
from Plugin cli tool to write to the same terminal that plugin cli tool writes to
by using the logging mechanism already in place.
The logging conf is not read at all pb plugin cli tool.

As a side effect, the loging level for components that are called
from the plugin command such as the jar hell check can now be configured
with -Des.logger.level which makes it easier to debug the jar hell check.
plugin cli tool should not create empty log files
install groovy plugin before running script test
For minScore and terminateAfter we can just rely on defaults set to SearchSourceBuilder. Also simplified toString representation of CountRequestBuilder
Also restored a couple of old tests around search and count request builder that used to get wiped when calling toString.
s1monw and others added 20 commits October 14, 2015 10:56
…lure

Ensure searcher is release if wrapping fails
This commit makes sure that the plugin script looks at user, group and permissions of the elasticsearch bin dir and copies them over to the plugin bin subdirectory, whatever they are, so that they get properly setup depending on how elasticsearch was installed. We also make sure that execute permissions are added for files (we already did this before).

Relates to #11016
Closes #14088
* Add ability for plugins to declare additional permissions with a custom plugin-security.policy file and corresponding AccessController logic. See the plugin author's guide for more information.
* Add warning messages to users for extra plugin permissions in bin/plugin.
* When bin/plugin is run interactively (stdin is a controlling terminal and -b/--batch not supplied), require user confirmation.
* Improve unit test and IDE support for plugins with additional permissions by exposing plugin's metadata as a maven test resource.

Closes #14108

Squashed commit of the following:

commit cf8ace6
Author: Robert Muir <[email protected]>
Date:   Wed Oct 14 13:36:05 2015 -0400

    fix new unit test from master merge

commit 9be3c5a
Merge: 2f168b8 7368231
Author: Robert Muir <[email protected]>
Date:   Wed Oct 14 12:58:31 2015 -0400

    Merge branch 'master' into off_my_back

commit 2f168b8
Author: Robert Muir <[email protected]>
Date:   Wed Oct 14 12:56:04 2015 -0400

    improve plugin author documentation

commit 6e6c2bf
Author: Robert Muir <[email protected]>
Date:   Wed Oct 14 12:52:14 2015 -0400

    move security confirmation after 'plugin already installed' check, to prevent user from answering unnecessary questions.

commit 08233a2
Author: Robert Muir <[email protected]>
Date:   Wed Oct 14 05:36:42 2015 -0400

    Add documentation and pluginmanager support

commit 05dad86
Author: Robert Muir <[email protected]>
Date:   Wed Oct 14 02:22:24 2015 -0400

    Decentralize plugin permissions (modulo docs and pluginmanager work)
This commit cleans up IndexMetaData. In particular, all duplicate
getters (X and getX) have been collapsed into one (getX). Further, the
number of shards and number of replicas settings are now parsed once
and saved off as fields.
This commit adds a new metric aggregator for computing the geo_centroid over a set of geo_point fields. This can be combined with other aggregators (e.g., geohash_grid, significant_terms) for computing the geospatial centroid based on the document sets from other aggregation results.
This test failed only on a feature branch, because that feature branch has
a different build randomization script and is the only place
randomizing tests.security.manager (this test cannot run with it enabled).

On old kernels without TSYNC support, the test fails because (surprise to me) the thread that
runs the test is not the same thread that runs static initializers:
https://github.com/randomizedtesting/randomizedtesting/blob/7571489190d677969c768836e1576f4e851f83e8/randomized-runner/src/main/java/com/carrotsearch/randomizedtesting/RandomizedRunner.java#L573-L574

To fix this test (its not an issue in practice, since we do this before creating threadpools),
we just record for testing purposes that we couldn't TSYNC, and re-run the whole thing for the test thread
in setUp(), failing if something goes wrong.

Also add a bunch of additional paranoia and narrow our defensive checks better here after reading
through more chrome bug reports: they don't impact us but those linux distros are too cowboy
with the backports and the spirit of the checks makes me feel better.
# Conflicts:
#	core/src/main/java/org/elasticsearch/search/SearchService.java
Fix SeccompTests bug on older kernels / add defense
Closes #14120

Squashed commit of the following:

commit 556b7f5
Author: Robert Muir <[email protected]>
Date:   Wed Oct 14 15:16:54 2015 -0400

    Add bugid link

commit b44aac7
Author: Robert Muir <[email protected]>
Date:   Wed Oct 14 15:01:41 2015 -0400

    Test that the lucene "unmap hack" is supported.

    We should know if this is not working for any configuration, otherwise resources such as address space, file handles, and even disk space become tied to Java's garbage collector.
@javanna javanna removed the WIP label Oct 19, 2015
@javanna
Copy link
Member

javanna commented Oct 19, 2015

This was merged, we can close. the only thing that's left to-do is documenting all of the specific breaking changes and eventually deprecate those methods removal in 2.x branch.

@javanna javanna closed this Oct 19, 2015
@danielmitterdorfer danielmitterdorfer deleted the feature/search-request-refactoring branch January 18, 2016 07:50
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Search Refactoring labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking >enhancement :Search/Search Search-related issues that do not fall into other categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.