From 7fad7c675d73a5386d3bdea475c85c083dd68f07 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Fri, 7 Apr 2017 11:46:41 -0400 Subject: [PATCH 01/26] Rewrite the scripting security docs (#23930) They needed to be updated now that Painless is the default and the non-sandboxed scripting languages are going away or gone. I dropped the entire section about customizing the classloader whitelists. In master this barely does anything (exposes more things to expressions). --- docs/build.gradle | 1 - docs/plugins/authors.asciidoc | 2 +- .../modules/scripting/security.asciidoc | 269 +++++++----------- .../reference/search/search-template.asciidoc | 4 +- 4 files changed, 100 insertions(+), 176 deletions(-) diff --git a/docs/build.gradle b/docs/build.gradle index ea63016e3bd..63d9d12c2da 100644 --- a/docs/build.gradle +++ b/docs/build.gradle @@ -87,7 +87,6 @@ buildRestTests.expectedUnconvertedCandidates = [ 'reference/mapping/types/nested.asciidoc', 'reference/mapping/types/object.asciidoc', 'reference/mapping/types/percolator.asciidoc', - 'reference/modules/scripting/security.asciidoc', 'reference/modules/cross-cluster-search.asciidoc', // this is hard to test since we need 2 clusters -- maybe we can trick it into referencing itself... 'reference/search/field-stats.asciidoc', 'reference/search/profile.asciidoc', diff --git a/docs/plugins/authors.asciidoc b/docs/plugins/authors.asciidoc index b7fd43f71c7..3b91fb76f66 100644 --- a/docs/plugins/authors.asciidoc +++ b/docs/plugins/authors.asciidoc @@ -76,6 +76,7 @@ Read more in {ref}/integration-tests.html#changing-node-configuration[Changing N [float] +[[plugin-authors-jsm]] === Java Security permissions Some plugins may need additional security permissions. A plugin can include @@ -111,4 +112,3 @@ AccessController.doPrivileged( See http://www.oracle.com/technetwork/java/seccodeguide-139067.html[Secure Coding Guidelines for Java SE] for more information. - diff --git a/docs/reference/modules/scripting/security.asciidoc b/docs/reference/modules/scripting/security.asciidoc index 110e65e52c8..be1806175c1 100644 --- a/docs/reference/modules/scripting/security.asciidoc +++ b/docs/reference/modules/scripting/security.asciidoc @@ -1,73 +1,115 @@ [[modules-scripting-security]] === Scripting and security -You should never run Elasticsearch as the `root` user, as this would allow a -script to access or do *anything* on your server, without limitations. +While Elasticsearch contributors make every effort to prevent scripts from +running amok, security is something best done in +https://en.wikipedia.org/wiki/Defense_in_depth_(computing)[layers] because +all software has bugs and it is important to minimize the risk of failure in +any security layer. Find below rules of thumb for how to keep Elasticsearch +from being a vulnerability. -You should not expose Elasticsearch directly to users, but instead have a -proxy application inbetween. If you *do* intend to expose Elasticsearch -directly to your users, then you have to decide whether you trust them enough -to run scripts on your box or not, and apply the appropriate safety measures. - -[[enable-dynamic-scripting]] [float] -=== Enabling dynamic scripting +=== Do not run as root +First and foremost, never run Elasticsearch as the `root` user as this would +allow any successful effort to circumvent the other security layers to do +*anything* on your server. Elasticsearch will refuse to start if it detects +that it is running as `root` but this is so important that it is worth double +and triple checking. -The `script.*` settings allow for <> -control of which script languages (e.g `painless`) are allowed to -run in which context ( e.g. `search`, `aggs`, `update`), and where the script -source is allowed to come from (i.e. `inline`, `stored`, `file`). +[float] +=== Do not expose Elasticsearch directly to users +Do not expose Elasticsearch directly to users, instead have an application +make requests on behalf of users. If this is not possible, have an application +to sanitize requests from users. If *that* is not possible then have some +mechanism to track which users did what. Understand that it is quite possible +to write a <> that overwhelms Elasticsearch and brings down +the cluster. All such searches should be considered bugs and the Elasticsearch +contributors make an effort to prevent this but they are still possible. -For instance, the following setting enables `stored` `update` scripts for -`painless`: +[float] +=== Do not expose Elasticsearch directly to the Internet +Do not expose Elasticsearch to the Internet, instead have an application +make requests on behalf of the Internet. Do not entertain the thought of having +an application "sanitize" requests to Elasticsearch. Understand that it is +possible for a sufficiently determined malicious user to write searches that +overwhelm the Elasticsearch cluster and bring it down. For example: -[source,yaml] ----------------- -script.engine.painless.inline.update: true ----------------- +Good: +* Users type text into a search box and the text is sent directly to a +<>, <>, +<>, or any of the <>. +* Running a script with any of the above queries that was written as part of +the application development process. +* Running a script with `params` provided by users. +* User actions makes documents with a fixed structure. -Less fine-grained settings exist which allow you to enable or disable scripts -for all sources, all languages, or all contexts. The following settings -enable `inline` and `stored` scripts for all languages in all contexts: +Bad: +* Users can write arbitrary scripts, queries, `_search` requests. +* User actions make documents with structure defined by users. -[source,yaml] ------------------------------------ -script.inline: true -script.stored: true ------------------------------------ +[float] +[[modules-scripting-security-do-no-weaken]] +=== Do not weaken script security settings +By default Elasticsearch will run inline, stored, and filesystem scripts for +sandboxed languages, namely the scripting language Painless, the template +language Mustache, and the expression language Expressions. These *ought* to be +safe to expose to trusted users and to your application servers because they +have strong security sandboxes. By default Elasticsearch will only run +filesystem scripts for non-sandboxed languages and enabling them is a poor +choice because: +1. This drops a layer of security, leaving only Elasticsearch's builtin +<>. +2. Non-sandboxed scripts have unchecked access to Elasticsearch's internals and +can cause all kinds of trouble if misused. -WARNING: The above settings mean that anybody who can send requests to your -Elasticsearch instance can run whatever scripts they choose! This is a -security risk and may well lead to your Elasticsearch cluster being -compromised. + +[float] +[[modules-scripting-other-layers]] +=== Other security layers +In addition to user privileges and script sandboxing Elasticsearch uses the +http://www.oracle.com/technetwork/java/seccodeguide-139067.html[Java Security Manager] +and native security tools as additional layers of security. + +As part of its startup sequence Elasticsearch enables the Java Security Manager +which limits the actions that can be taken by portions of the code. Painless +uses this to limit the actions that generated Painless scripts can take, +preventing them from being able to do things like write files and listen to +sockets. + +Elasticsearch uses +https://en.wikipedia.org/wiki/Seccomp[seccomp] in Linux, +https://www.chromium.org/developers/design-documents/sandbox/osx-sandboxing-design[Seatbelt] +in macOS, and +https://msdn.microsoft.com/en-us/library/windows/desktop/ms684147[ActiveProcessLimit] +on Windows to prevent Elasticsearch from forking or executing other processes. + +Below this we describe the security settings for scripts and how you can +change from the defaults described above. You should be very, very careful +when allowing more than the defaults. Any extra permissions weakens the total +security of the Elasticsearch deployment. [[security-script-source]] [float] === Script source settings -Scripts may be enabled or disabled depending on their source: `inline`, -`stored` in the cluster state, or from a `file` on each node in the cluster. -Each of these settings takes one of these values: - - -[horizontal] -`false`:: Scripting is disabled. -`true`:: Scripting is enabled. - -The default values are the following: +Which scripts Elasticsearch will execute where is controlled by settings +starting with `scripts.`. The simplest settings allow scripts to be enabled +or disabled based on where they are stored. For example: [source,yaml] ----------------------------------- -script.inline: false -script.stored: false -script.file: true +script.inline: false <1> +script.stored: false <2> +script.file: true <3> ----------------------------------- +<1> Refuse to run scripts provided inline in the API. +<2> Refuse to run scripts stored using the API. +<3> Run scripts found on the filesystem in `/etc/elasticsearch/scripts` +(rpm or deb) or `config/scripts` (zip or tar). -NOTE: Global scripting settings affect the `mustache` scripting language. -<> internally use the `mustache` language, -and will still be enabled by default as the `mustache` engine is sandboxed, -but they will be enabled/disabled according to fine-grained settings -specified in `elasticsearch.yml`. +NOTE: These settings override the defaults mentioned +<>. Recreating the defaults +requires more fine grained settings described <>. [[security-script-context]] [float] @@ -102,15 +144,13 @@ script.plugin: false === Fine-grained script settings First, the high-level script settings described above are applied in order -(context settings have precedence over source settings). Then, fine-grained +(context settings have precedence over source settings). Then fine-grained settings which include the script language take precedence over any high-level -settings. - -Fine-grained settings have the form: +settings. They have two forms: [source,yaml] ------------------------ -script.engine.{lang}.{source}.{context}: true|false +script.engine.{lang}.{inline|file|stored}.{context}: true|false ------------------------ And @@ -132,124 +172,9 @@ script.engine.painless.inline: true <2> script.engine.painless.stored.search: true <3> script.engine.painless.stored.aggs: true <3> -script.engine.mustache.stored.search: true <4> +script.engine.mustache.stored.search: true <4> ----------------------------------- <1> Disable all scripting from any source. -<2> Allow inline Groovy scripts for all operations -<3> Allow stored Groovy scripts to be used for search and aggregations. +<2> Allow inline Painless scripts for all operations. +<3> Allow stored Painless scripts to be used for search and aggregations. <4> Allow stored Mustache templates to be used for search. - -[[java-security-manager]] -[float] -=== Java Security Manager - -Elasticsearch runs with the https://docs.oracle.com/javase/tutorial/essential/environment/security.html[Java Security Manager] -enabled by default. The security policy in Elasticsearch locks down the -permissions granted to each class to the bare minimum required to operate. -The benefit of doing this is that it severely limits the attack vectors -available to a hacker. - -Restricting permissions is particularly important with scripting languages -like Groovy which is designed to do anything that can be done -in Java itself, including writing to the file system, opening sockets to -remote servers, etc. - -[float] -=== Script Classloader Whitelist - -Scripting languages are only allowed to load classes which appear in a -hardcoded whitelist that can be found in -https://github.com/elastic/elasticsearch/blob/{branch}/core/src/main/java/org/elasticsearch/script/ClassPermission.java[`org.elasticsearch.script.ClassPermission`]. - - -In a script, attempting to load a class that does not appear in the whitelist -_may_ result in a `ClassNotFoundException`, for instance this script: - -[source,js] ------------------------------- -GET _search -{ - "script_fields": { - "the_hour": { - "script": "use(java.math.BigInteger); new BigInteger(1)" - } - } -} ------------------------------- - -will return the following exception: - -[source,js] ------------------------------- -{ - "reason": { - "type": "script_exception", - "reason": "failed to run inline script [use(java.math.BigInteger); new BigInteger(1)] using lang [painless]", - "caused_by": { - "type": "no_class_def_found_error", - "reason": "java/math/BigInteger", - "caused_by": { - "type": "class_not_found_exception", - "reason": "java.math.BigInteger" - } - } - } -} ------------------------------- - -[float] -== Dealing with Java Security Manager issues - -If you encounter issues with the Java Security Manager, you have two options -for resolving these issues: - -[float] -=== Fix the security problem - -The safest and most secure long term solution is to change the code causing -the security issue. We recognise that this may take time to do correctly and -so we provide the following two alternatives. - -[float] -=== Customising the classloader whitelist - -The classloader whitelist can be customised by tweaking the local Java -Security Policy either: - -* system wide: `$JAVA_HOME/lib/security/java.policy`, -* for just the `elasticsearch` user: `/home/elasticsearch/.java.policy` -* by adding a system property to the <> configuration: `-Djava.security.policy=someURL`, or -* via the `ES_JAVA_OPTS` environment variable with `-Djava.security.policy=someURL`: -+ -[source,js] ---------------------------------- -export ES_JAVA_OPTS="${ES_JAVA_OPTS} -Djava.security.policy=file:///path/to/my.policy` -./bin/elasticsearch ---------------------------------- - -Permissions may be granted at the class, package, or global level. For instance: - -[source,js] ----------------------------------- -grant { - permission org.elasticsearch.script.ClassPermission "java.util.Base64"; // allow class - permission org.elasticsearch.script.ClassPermission "java.util.*"; // allow package - permission org.elasticsearch.script.ClassPermission "*"; // allow all (disables filtering basically) -}; ----------------------------------- - -[TIP] -====================================== - -Before adding classes to the whitelist, consider the security impact that it -will have on Elasticsearch. Do you really need an extra class or can your code -be rewritten in a more secure way? - -It is quite possible that we have not whitelisted a generically useful and -safe class. If you have a class that you think should be whitelisted by -default, please open an issue on GitHub and we will consider the impact of -doing so. - -====================================== - -See http://docs.oracle.com/javase/7/docs/technotes/guides/security/PolicyFiles.html for more information. diff --git a/docs/reference/search/search-template.asciidoc b/docs/reference/search/search-template.asciidoc index b2412cc1cb6..d8f0fbb9719 100644 --- a/docs/reference/search/search-template.asciidoc +++ b/docs/reference/search/search-template.asciidoc @@ -28,8 +28,8 @@ documentation of the mustache project]. NOTE: The mustache language is implemented in elasticsearch as a sandboxed scripting language, hence it obeys settings that may be used to enable or -disable scripts per language, source and operation as described in -<> +disable scripts per language, source and operation as described in the +<> [float] ==== More template examples From 776006bac58d43cbcbe72e32343540ce9fd7dc88 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 7 Apr 2017 11:27:26 -0700 Subject: [PATCH 02/26] Collapse repository gcs classes into a single java package (#23975) This is a single reorge of the classes to simplify making them mostly package protected. --- .../src/main/resources/checkstyle_suppressions.xml | 14 +++++++------- plugins/repository-gcs/build.gradle | 2 +- .../gcs/GoogleCloudStorageBlobContainer.java | 4 ++-- .../gcs/GoogleCloudStorageBlobStore.java | 11 +++-------- .../gcs/GoogleCloudStoragePlugin.java | 2 +- .../gcs/GoogleCloudStorageRepository.java | 6 ++---- .../gcs/GoogleCloudStorageService.java | 4 ++-- .../util => repositories/gcs}/SocketAccess.java | 4 ++-- .../GoogleCloudStorageBlobStoreContainerTests.java | 2 +- ...GoogleCloudStorageBlobStoreRepositoryTests.java | 2 -- .../gcs/GoogleCloudStorageBlobStoreTests.java | 2 +- .../gcs/MockHttpTransport.java | 2 +- 12 files changed, 23 insertions(+), 32 deletions(-) rename plugins/repository-gcs/src/main/java/org/elasticsearch/{common/blobstore => repositories}/gcs/GoogleCloudStorageBlobContainer.java (95%) rename plugins/repository-gcs/src/main/java/org/elasticsearch/{common/blobstore => repositories}/gcs/GoogleCloudStorageBlobStore.java (96%) rename plugins/repository-gcs/src/main/java/org/elasticsearch/{plugin/repository => repositories}/gcs/GoogleCloudStoragePlugin.java (99%) rename plugins/repository-gcs/src/main/java/org/elasticsearch/{common/blobstore/gcs/util => repositories/gcs}/SocketAccess.java (96%) rename plugins/repository-gcs/src/test/java/org/elasticsearch/{common/blobstore => repositories}/gcs/GoogleCloudStorageBlobStoreContainerTests.java (96%) rename plugins/repository-gcs/src/test/java/org/elasticsearch/{common/blobstore => repositories}/gcs/GoogleCloudStorageBlobStoreTests.java (96%) rename plugins/repository-gcs/src/test/java/org/elasticsearch/{common/blobstore => repositories}/gcs/MockHttpTransport.java (99%) diff --git a/buildSrc/src/main/resources/checkstyle_suppressions.xml b/buildSrc/src/main/resources/checkstyle_suppressions.xml index 11882b2faa9..f50d0422380 100644 --- a/buildSrc/src/main/resources/checkstyle_suppressions.xml +++ b/buildSrc/src/main/resources/checkstyle_suppressions.xml @@ -3892,15 +3892,15 @@ - - - - + + + + - - - + + + diff --git a/plugins/repository-gcs/build.gradle b/plugins/repository-gcs/build.gradle index 9968d4408e4..84316827a20 100644 --- a/plugins/repository-gcs/build.gradle +++ b/plugins/repository-gcs/build.gradle @@ -19,7 +19,7 @@ esplugin { description 'The GCS repository plugin adds Google Cloud Storage support for repositories.' - classname 'org.elasticsearch.plugin.repository.gcs.GoogleCloudStoragePlugin' + classname 'org.elasticsearch.repositories.gcs.GoogleCloudStoragePlugin' } versions << [ diff --git a/plugins/repository-gcs/src/main/java/org/elasticsearch/common/blobstore/gcs/GoogleCloudStorageBlobContainer.java b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobContainer.java similarity index 95% rename from plugins/repository-gcs/src/main/java/org/elasticsearch/common/blobstore/gcs/GoogleCloudStorageBlobContainer.java rename to plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobContainer.java index 92ade4be4f9..331e2dadca2 100644 --- a/plugins/repository-gcs/src/main/java/org/elasticsearch/common/blobstore/gcs/GoogleCloudStorageBlobContainer.java +++ b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobContainer.java @@ -17,7 +17,7 @@ * under the License. */ -package org.elasticsearch.common.blobstore.gcs; +package org.elasticsearch.repositories.gcs; import org.elasticsearch.common.blobstore.BlobMetaData; import org.elasticsearch.common.blobstore.BlobPath; @@ -29,7 +29,7 @@ import java.io.InputStream; import java.nio.file.FileAlreadyExistsException; import java.util.Map; -public class GoogleCloudStorageBlobContainer extends AbstractBlobContainer { +class GoogleCloudStorageBlobContainer extends AbstractBlobContainer { private final GoogleCloudStorageBlobStore blobStore; private final String path; diff --git a/plugins/repository-gcs/src/main/java/org/elasticsearch/common/blobstore/gcs/GoogleCloudStorageBlobStore.java b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java similarity index 96% rename from plugins/repository-gcs/src/main/java/org/elasticsearch/common/blobstore/gcs/GoogleCloudStorageBlobStore.java rename to plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java index 01b8d510be9..3c50c3a2a2d 100644 --- a/plugins/repository-gcs/src/main/java/org/elasticsearch/common/blobstore/gcs/GoogleCloudStorageBlobStore.java +++ b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java @@ -17,7 +17,7 @@ * under the License. */ -package org.elasticsearch.common.blobstore.gcs; +package org.elasticsearch.repositories.gcs; import com.google.api.client.googleapis.batch.BatchRequest; import com.google.api.client.googleapis.batch.json.JsonBatchCallback; @@ -29,14 +29,12 @@ import com.google.api.services.storage.Storage; import com.google.api.services.storage.model.Bucket; import com.google.api.services.storage.model.Objects; import com.google.api.services.storage.model.StorageObject; -import org.elasticsearch.SpecialPermission; import org.elasticsearch.common.Strings; import org.elasticsearch.common.blobstore.BlobContainer; import org.elasticsearch.common.blobstore.BlobMetaData; import org.elasticsearch.common.blobstore.BlobPath; import org.elasticsearch.common.blobstore.BlobStore; import org.elasticsearch.common.blobstore.BlobStoreException; -import org.elasticsearch.common.blobstore.gcs.util.SocketAccess; import org.elasticsearch.common.blobstore.support.PlainBlobMetaData; import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.settings.Settings; @@ -45,9 +43,6 @@ import org.elasticsearch.common.util.concurrent.CountDown; import java.io.IOException; import java.io.InputStream; import java.nio.file.NoSuchFileException; -import java.security.AccessController; -import java.security.PrivilegedActionException; -import java.security.PrivilegedExceptionAction; import java.util.ArrayList; import java.util.Collection; import java.util.Iterator; @@ -62,7 +57,7 @@ import java.util.stream.StreamSupport; import static java.net.HttpURLConnection.HTTP_NOT_FOUND; -public class GoogleCloudStorageBlobStore extends AbstractComponent implements BlobStore { +class GoogleCloudStorageBlobStore extends AbstractComponent implements BlobStore { /** * Google Cloud Storage batch requests are limited to 1000 operations @@ -72,7 +67,7 @@ public class GoogleCloudStorageBlobStore extends AbstractComponent implements Bl private final Storage client; private final String bucket; - public GoogleCloudStorageBlobStore(Settings settings, String bucket, Storage storageClient) { + GoogleCloudStorageBlobStore(Settings settings, String bucket, Storage storageClient) { super(settings); this.bucket = bucket; this.client = storageClient; diff --git a/plugins/repository-gcs/src/main/java/org/elasticsearch/plugin/repository/gcs/GoogleCloudStoragePlugin.java b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStoragePlugin.java similarity index 99% rename from plugins/repository-gcs/src/main/java/org/elasticsearch/plugin/repository/gcs/GoogleCloudStoragePlugin.java rename to plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStoragePlugin.java index b20ed34e532..b1bacab22fd 100644 --- a/plugins/repository-gcs/src/main/java/org/elasticsearch/plugin/repository/gcs/GoogleCloudStoragePlugin.java +++ b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStoragePlugin.java @@ -17,7 +17,7 @@ * under the License. */ -package org.elasticsearch.plugin.repository.gcs; +package org.elasticsearch.repositories.gcs; import java.security.AccessController; import java.security.PrivilegedAction; diff --git a/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRepository.java b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRepository.java index b7473a2d5fa..79cbbe5b156 100644 --- a/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRepository.java +++ b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRepository.java @@ -24,14 +24,12 @@ import org.elasticsearch.cluster.metadata.RepositoryMetaData; import org.elasticsearch.common.Strings; import org.elasticsearch.common.blobstore.BlobPath; import org.elasticsearch.common.blobstore.BlobStore; -import org.elasticsearch.common.blobstore.gcs.GoogleCloudStorageBlobStore; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.unit.ByteSizeUnit; import org.elasticsearch.common.unit.ByteSizeValue; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.env.Environment; -import org.elasticsearch.plugin.repository.gcs.GoogleCloudStoragePlugin; import org.elasticsearch.repositories.RepositoryException; import org.elasticsearch.repositories.blobstore.BlobStoreRepository; @@ -44,7 +42,7 @@ import static org.elasticsearch.common.settings.Setting.simpleString; import static org.elasticsearch.common.settings.Setting.timeSetting; import static org.elasticsearch.common.unit.TimeValue.timeValueMillis; -public class GoogleCloudStorageRepository extends BlobStoreRepository { +class GoogleCloudStorageRepository extends BlobStoreRepository { // package private for testing static final ByteSizeValue MIN_CHUNK_SIZE = new ByteSizeValue(1, ByteSizeUnit.BYTES); @@ -76,7 +74,7 @@ public class GoogleCloudStorageRepository extends BlobStoreRepository { private final BlobPath basePath; private final GoogleCloudStorageBlobStore blobStore; - public GoogleCloudStorageRepository(RepositoryMetaData metadata, Environment environment, + GoogleCloudStorageRepository(RepositoryMetaData metadata, Environment environment, NamedXContentRegistry namedXContentRegistry, GoogleCloudStorageService storageService) throws Exception { super(metadata, environment.settings(), namedXContentRegistry); diff --git a/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageService.java b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageService.java index 55e7813de42..d541a729a71 100644 --- a/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageService.java +++ b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageService.java @@ -45,7 +45,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.Collections; -public interface GoogleCloudStorageService { +interface GoogleCloudStorageService { /** * Creates a client that can be used to manage Google Cloud Storage objects. @@ -67,7 +67,7 @@ public interface GoogleCloudStorageService { private final Environment environment; - public InternalGoogleCloudStorageService(Environment environment) { + InternalGoogleCloudStorageService(Environment environment) { super(environment.settings()); this.environment = environment; } diff --git a/plugins/repository-gcs/src/main/java/org/elasticsearch/common/blobstore/gcs/util/SocketAccess.java b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/SocketAccess.java similarity index 96% rename from plugins/repository-gcs/src/main/java/org/elasticsearch/common/blobstore/gcs/util/SocketAccess.java rename to plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/SocketAccess.java index 5d2b4c6bb21..27711d18d68 100644 --- a/plugins/repository-gcs/src/main/java/org/elasticsearch/common/blobstore/gcs/util/SocketAccess.java +++ b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/SocketAccess.java @@ -17,7 +17,7 @@ * under the License. */ -package org.elasticsearch.common.blobstore.gcs.util; +package org.elasticsearch.repositories.gcs; import org.elasticsearch.SpecialPermission; import org.elasticsearch.common.CheckedRunnable; @@ -34,7 +34,7 @@ import java.security.PrivilegedExceptionAction; * needs {@link SocketPermission} 'connect' to establish connections. This class wraps the operations requiring access * in {@link AccessController#doPrivileged(PrivilegedAction)} blocks. */ -public final class SocketAccess { +final class SocketAccess { private SocketAccess() {} diff --git a/plugins/repository-gcs/src/test/java/org/elasticsearch/common/blobstore/gcs/GoogleCloudStorageBlobStoreContainerTests.java b/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreContainerTests.java similarity index 96% rename from plugins/repository-gcs/src/test/java/org/elasticsearch/common/blobstore/gcs/GoogleCloudStorageBlobStoreContainerTests.java rename to plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreContainerTests.java index f7c6fc2eac9..f3fada5a463 100644 --- a/plugins/repository-gcs/src/test/java/org/elasticsearch/common/blobstore/gcs/GoogleCloudStorageBlobStoreContainerTests.java +++ b/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreContainerTests.java @@ -17,7 +17,7 @@ * under the License. */ -package org.elasticsearch.common.blobstore.gcs; +package org.elasticsearch.repositories.gcs; import org.elasticsearch.common.blobstore.BlobStore; import org.elasticsearch.common.settings.Settings; diff --git a/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java b/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java index eeb877dabf5..b2ff8bfe1fd 100644 --- a/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java +++ b/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java @@ -21,13 +21,11 @@ package org.elasticsearch.repositories.gcs; import com.google.api.services.storage.Storage; import org.elasticsearch.cluster.metadata.RepositoryMetaData; -import org.elasticsearch.common.blobstore.gcs.MockHttpTransport; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.ByteSizeUnit; import org.elasticsearch.common.unit.ByteSizeValue; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.env.Environment; -import org.elasticsearch.plugin.repository.gcs.GoogleCloudStoragePlugin; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.repositories.blobstore.ESBlobStoreRepositoryIntegTestCase; import org.junit.BeforeClass; diff --git a/plugins/repository-gcs/src/test/java/org/elasticsearch/common/blobstore/gcs/GoogleCloudStorageBlobStoreTests.java b/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreTests.java similarity index 96% rename from plugins/repository-gcs/src/test/java/org/elasticsearch/common/blobstore/gcs/GoogleCloudStorageBlobStoreTests.java rename to plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreTests.java index 65add371855..fe94237f6c9 100644 --- a/plugins/repository-gcs/src/test/java/org/elasticsearch/common/blobstore/gcs/GoogleCloudStorageBlobStoreTests.java +++ b/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreTests.java @@ -17,7 +17,7 @@ * under the License. */ -package org.elasticsearch.common.blobstore.gcs; +package org.elasticsearch.repositories.gcs; import org.elasticsearch.common.blobstore.BlobStore; import org.elasticsearch.common.settings.Settings; diff --git a/plugins/repository-gcs/src/test/java/org/elasticsearch/common/blobstore/gcs/MockHttpTransport.java b/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/MockHttpTransport.java similarity index 99% rename from plugins/repository-gcs/src/test/java/org/elasticsearch/common/blobstore/gcs/MockHttpTransport.java rename to plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/MockHttpTransport.java index ffb9c8a910f..a9d358c831f 100644 --- a/plugins/repository-gcs/src/test/java/org/elasticsearch/common/blobstore/gcs/MockHttpTransport.java +++ b/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/MockHttpTransport.java @@ -17,7 +17,7 @@ * under the License. */ -package org.elasticsearch.common.blobstore.gcs; +package org.elasticsearch.repositories.gcs; import com.google.api.client.http.HttpTransport; import com.google.api.client.http.LowLevelHttpRequest; From 457a76c1c6becab4e9ae04f48c470edcd5687ae0 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 7 Apr 2017 14:51:42 -0400 Subject: [PATCH 03/26] Fix import order in Spawner The imports are not in alphabetical order in Spawner.java and this is a crime that is rectified by this commit. --- core/src/main/java/org/elasticsearch/bootstrap/Spawner.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Spawner.java b/core/src/main/java/org/elasticsearch/bootstrap/Spawner.java index 53983cb472e..77cadaa9043 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/Spawner.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/Spawner.java @@ -21,8 +21,8 @@ package org.elasticsearch.bootstrap; import org.apache.lucene.util.IOUtils; import org.elasticsearch.env.Environment; -import org.elasticsearch.plugins.PluginInfo; import org.elasticsearch.plugins.Platforms; +import org.elasticsearch.plugins.PluginInfo; import java.io.Closeable; import java.io.IOException; From 0c465b1931438c4e83634235eb483f5cd329923a Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Fri, 7 Apr 2017 21:00:04 +0200 Subject: [PATCH 04/26] Add comment why we check for null fetch results during merge --- .../action/search/SearchPhaseController.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/core/src/main/java/org/elasticsearch/action/search/SearchPhaseController.java b/core/src/main/java/org/elasticsearch/action/search/SearchPhaseController.java index 9972b6e1c2a..38d793b93ed 100644 --- a/core/src/main/java/org/elasticsearch/action/search/SearchPhaseController.java +++ b/core/src/main/java/org/elasticsearch/action/search/SearchPhaseController.java @@ -304,6 +304,10 @@ public final class SearchPhaseController extends AbstractComponent { ScoreDoc shardDoc = sortedDocs[scoreDocIndex]; SearchPhaseResult searchResultProvider = resultsLookup.apply(shardDoc.shardIndex); if (searchResultProvider == null) { + // this can happen if we are hitting a shard failure during the fetch phase + // in this case we referenced the shard result via teh ScoreDoc but never got a + // result from fetch. + // TODO it would be nice to assert this in the future continue; } FetchSearchResult fetchResult = searchResultProvider.fetchResult(); @@ -358,6 +362,10 @@ public final class SearchPhaseController extends AbstractComponent { ScoreDoc shardDoc = sortedDocs[i]; SearchPhaseResult fetchResultProvider = resultsLookup.apply(shardDoc.shardIndex); if (fetchResultProvider == null) { + // this can happen if we are hitting a shard failure during the fetch phase + // in this case we referenced the shard result via teh ScoreDoc but never got a + // result from fetch. + // TODO it would be nice to assert this in the future continue; } FetchSearchResult fetchResult = fetchResultProvider.fetchResult(); From de6837b7ac7f99555d128b9e390fe70c84893934 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Fri, 7 Apr 2017 15:56:52 -0400 Subject: [PATCH 05/26] Fix throttled reindex_from_remote (#23953) reindex_from_remote was using `TimeValue#toString` to generate the scroll timeout which is bad because that generates fractional time values that are useful for people but bad for Elasticsearch which doesn't like to parse them. This switches it to using `TimeValue#getStringRep` which spits out whole time values. Closes to #23945 Makes #23828 even more desirable --- .../reindex/remote/RemoteRequestBuilders.java | 4 +- .../remote/RemoteRequestBuildersTests.java | 10 ++- .../rest-api-spec/test/reindex/90_remote.yaml | 84 +++++++++++++++++++ 3 files changed, 93 insertions(+), 5 deletions(-) diff --git a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuilders.java b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuilders.java index 4804818890e..036de6f0e22 100644 --- a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuilders.java +++ b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuilders.java @@ -59,7 +59,7 @@ final class RemoteRequestBuilders { static Map initialSearchParams(SearchRequest searchRequest, Version remoteVersion) { Map params = new HashMap<>(); if (searchRequest.scroll() != null) { - params.put("scroll", searchRequest.scroll().keepAlive().toString()); + params.put("scroll", searchRequest.scroll().keepAlive().getStringRep()); } params.put("size", Integer.toString(searchRequest.source().size())); if (searchRequest.source().version() == null || searchRequest.source().version() == true) { @@ -168,7 +168,7 @@ final class RemoteRequestBuilders { } static Map scrollParams(TimeValue keepAlive) { - return singletonMap("scroll", keepAlive.toString()); + return singletonMap("scroll", keepAlive.getStringRep()); } static HttpEntity scrollEntity(String scroll, Version remoteVersion) { diff --git a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuildersTests.java b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuildersTests.java index 16f0c2a0a4e..779c89f7ee8 100644 --- a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuildersTests.java +++ b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuildersTests.java @@ -35,11 +35,11 @@ import java.io.InputStreamReader; import java.nio.charset.StandardCharsets; import java.util.Map; +import static org.elasticsearch.index.reindex.remote.RemoteRequestBuilders.clearScrollEntity; import static org.elasticsearch.index.reindex.remote.RemoteRequestBuilders.initialSearchEntity; import static org.elasticsearch.index.reindex.remote.RemoteRequestBuilders.initialSearchParams; import static org.elasticsearch.index.reindex.remote.RemoteRequestBuilders.initialSearchPath; import static org.elasticsearch.index.reindex.remote.RemoteRequestBuilders.scrollEntity; -import static org.elasticsearch.index.reindex.remote.RemoteRequestBuilders.clearScrollEntity; import static org.elasticsearch.index.reindex.remote.RemoteRequestBuilders.scrollParams; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.either; @@ -150,7 +150,11 @@ public class RemoteRequestBuildersTests extends ESTestCase { Map params = initialSearchParams(searchRequest, remoteVersion); - assertThat(params, scroll == null ? not(hasKey("scroll")) : hasEntry("scroll", scroll.toString())); + if (scroll == null) { + assertThat(params, not(hasKey("scroll"))); + } else { + assertEquals(scroll, TimeValue.parseTimeValue(params.get("scroll"), "scroll")); + } assertThat(params, hasEntry("size", Integer.toString(size))); assertThat(params, fetchVersion == null || fetchVersion == true ? hasEntry("version", null) : not(hasEntry("version", null))); } @@ -181,7 +185,7 @@ public class RemoteRequestBuildersTests extends ESTestCase { public void testScrollParams() { TimeValue scroll = TimeValue.parseTimeValue(randomPositiveTimeValue(), "test"); - assertThat(scrollParams(scroll), hasEntry("scroll", scroll.toString())); + assertEquals(scroll, TimeValue.parseTimeValue(scrollParams(scroll).get("scroll"), "scroll")); } public void testScrollEntity() throws IOException { diff --git a/modules/reindex/src/test/resources/rest-api-spec/test/reindex/90_remote.yaml b/modules/reindex/src/test/resources/rest-api-spec/test/reindex/90_remote.yaml index 576447b4e54..8690d329d38 100644 --- a/modules/reindex/src/test/resources/rest-api-spec/test/reindex/90_remote.yaml +++ b/modules/reindex/src/test/resources/rest-api-spec/test/reindex/90_remote.yaml @@ -459,3 +459,87 @@ id: 1 - match: { _source.text: "test" } - is_false: _source.filtered + +--- +"Reindex from remote with rethrottle": + # Throttling happens between each scroll batch so we need to control the size of the batch by using a single shard + # and a small batch size on the request + - do: + indices.create: + index: source + body: + settings: + number_of_shards: "1" + number_of_replicas: "0" + - do: + index: + index: source + type: foo + id: 1 + body: { "text": "test" } + - do: + index: + index: source + type: foo + id: 2 + body: { "text": "test" } + - do: + index: + index: source + type: foo + id: 3 + body: { "text": "test" } + - do: + indices.refresh: {} + + + # Fetch the http host. We use the host of the master because we know there will always be a master. + - do: + cluster.state: {} + - set: { master_node: master } + - do: + nodes.info: + metric: [ http ] + - is_true: nodes.$master.http.publish_address + - set: {nodes.$master.http.publish_address: host} + - do: + reindex: + requests_per_second: .00000001 # About 9.5 years to complete the request + wait_for_completion: false + refresh: true + body: + source: + remote: + host: http://${host} + index: source + size: 1 + dest: + index: dest + - match: {task: '/.+:\d+/'} + - set: {task: task} + + - do: + reindex_rethrottle: + requests_per_second: -1 + task_id: $task + + - do: + tasks.get: + wait_for_completion: true + task_id: $task + + - do: + search: + index: dest + body: + query: + match: + text: test + - match: {hits.total: 3} + + # Make sure reindex closed all the scroll contexts + - do: + indices.stats: + index: source + metric: search + - match: {indices.source.total.search.open_contexts: 0} From d4c0ef00285fff28c83fe0f0d1cfaf10165632ae Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 7 Apr 2017 13:28:15 -0700 Subject: [PATCH 06/26] Settings: Migrate ec2 discovery sensitive settings to elasticsearch keystore (#23961) This change adds secure settings for access/secret keys and proxy username/password to ec2 discovery. It adds the new settings with the prefix `discovery.ec2`, copies other relevant ec2 client settings to the same prefix, and deprecates all other settings (`cloud.aws.*` and `cloud.aws.ec2.*`). Note that this is simpler than the client configs in repository-s3 because discovery is only initialized once for the entire node, so there is no reason to complicate the configuration with the ability to have multiple sets of client settings. relates #22475 --- .../discovery/ec2/AwsEc2Service.java | 97 +++++++++++++------ .../discovery/ec2/AwsEc2ServiceImpl.java | 47 ++++----- .../discovery/ec2/AwsEc2ServiceImplTests.java | 88 ++++++++++++++++- .../ec2/Ec2DiscoverySettingsTests.java | 33 +++++++ .../org/elasticsearch/test/ESTestCase.java | 2 +- 5 files changed, 214 insertions(+), 53 deletions(-) diff --git a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2Service.java b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2Service.java index b246ec21de2..a1ca208e4ac 100644 --- a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2Service.java +++ b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2Service.java @@ -22,6 +22,8 @@ package org.elasticsearch.discovery.ec2; import com.amazonaws.ClientConfiguration; import com.amazonaws.Protocol; import com.amazonaws.services.ec2.AmazonEC2; +import org.elasticsearch.common.settings.SecureSetting; +import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.settings.Settings; @@ -42,50 +44,52 @@ interface AwsEc2Service { /** * cloud.aws.access_key: AWS Access key. Shared with repository-s3 plugin */ - Setting KEY_SETTING = - Setting.simpleString("cloud.aws.access_key", Property.NodeScope, Property.Filtered, Property.Shared); + Setting KEY_SETTING = new Setting<>("cloud.aws.access_key", "", SecureString::new, + Property.NodeScope, Property.Filtered, Property.Shared, Property.Deprecated); /** * cloud.aws.secret_key: AWS Secret key. Shared with repository-s3 plugin */ - Setting SECRET_SETTING = - Setting.simpleString("cloud.aws.secret_key", Property.NodeScope, Property.Filtered, Property.Shared); + Setting SECRET_SETTING = new Setting<>("cloud.aws.secret_key", "", SecureString::new, + Property.NodeScope, Property.Filtered, Property.Shared, Property.Deprecated); /** * cloud.aws.protocol: Protocol for AWS API: http or https. Defaults to https. Shared with repository-s3 plugin */ Setting PROTOCOL_SETTING = new Setting<>("cloud.aws.protocol", "https", s -> Protocol.valueOf(s.toUpperCase(Locale.ROOT)), - Property.NodeScope, Property.Shared); + Property.NodeScope, Property.Shared, Property.Deprecated); /** * cloud.aws.proxy.host: In case of proxy, define its hostname/IP. Shared with repository-s3 plugin */ - Setting PROXY_HOST_SETTING = Setting.simpleString("cloud.aws.proxy.host", Property.NodeScope, Property.Shared); + Setting PROXY_HOST_SETTING = Setting.simpleString("cloud.aws.proxy.host", + Property.NodeScope, Property.Shared, Property.Deprecated); /** * cloud.aws.proxy.port: In case of proxy, define its port. Defaults to 80. Shared with repository-s3 plugin */ - Setting PROXY_PORT_SETTING = Setting.intSetting("cloud.aws.proxy.port", 80, 0, 1<<16, Property.NodeScope, - Property.Shared); + Setting PROXY_PORT_SETTING = Setting.intSetting("cloud.aws.proxy.port", 80, 0, 1<<16, + Property.NodeScope, Property.Shared, Property.Deprecated); /** * cloud.aws.proxy.username: In case of proxy with auth, define the username. Shared with repository-s3 plugin */ - Setting PROXY_USERNAME_SETTING = Setting.simpleString("cloud.aws.proxy.username", Property.NodeScope, Property.Shared); + Setting PROXY_USERNAME_SETTING = new Setting<>("cloud.aws.proxy.username", "", SecureString::new, + Property.NodeScope, Property.Filtered, Property.Shared, Property.Deprecated); /** * cloud.aws.proxy.password: In case of proxy with auth, define the password. Shared with repository-s3 plugin */ - Setting PROXY_PASSWORD_SETTING = - Setting.simpleString("cloud.aws.proxy.password", Property.NodeScope, Property.Filtered, Property.Shared); + Setting PROXY_PASSWORD_SETTING = new Setting<>("cloud.aws.proxy.password", "", SecureString::new, + Property.NodeScope, Property.Filtered, Property.Shared, Property.Deprecated); /** * cloud.aws.signer: If you are using an old AWS API version, you can define a Signer. Shared with repository-s3 plugin */ - Setting SIGNER_SETTING = Setting.simpleString("cloud.aws.signer", Property.NodeScope, Property.Shared); + Setting SIGNER_SETTING = Setting.simpleString("cloud.aws.signer", Property.NodeScope, Property.Shared, Property.Deprecated); /** * cloud.aws.region: Region. Shared with repository-s3 plugin */ Setting REGION_SETTING = - new Setting<>("cloud.aws.region", "", s -> s.toLowerCase(Locale.ROOT), Property.NodeScope, Property.Shared); + new Setting<>("cloud.aws.region", "", s -> s.toLowerCase(Locale.ROOT), Property.NodeScope, Property.Shared, Property.Deprecated); /** * cloud.aws.read_timeout: Socket read timeout. Shared with repository-s3 plugin */ Setting READ_TIMEOUT = Setting.timeSetting("cloud.aws.read_timeout", - TimeValue.timeValueMillis(ClientConfiguration.DEFAULT_SOCKET_TIMEOUT), Property.NodeScope, Property.Shared); + TimeValue.timeValueMillis(ClientConfiguration.DEFAULT_SOCKET_TIMEOUT), Property.NodeScope, Property.Shared, Property.Deprecated); /** * Defines specific ec2 settings starting with cloud.aws.ec2. @@ -95,69 +99,70 @@ interface AwsEc2Service { * cloud.aws.ec2.access_key: AWS Access key specific for EC2 API calls. Defaults to cloud.aws.access_key. * @see AwsEc2Service#KEY_SETTING */ - Setting KEY_SETTING = new Setting<>("cloud.aws.ec2.access_key", AwsEc2Service.KEY_SETTING, Function.identity(), - Property.NodeScope, Property.Filtered); + Setting KEY_SETTING = new Setting<>("cloud.aws.ec2.access_key", AwsEc2Service.KEY_SETTING, + SecureString::new, Property.NodeScope, Property.Filtered, Property.Deprecated); + /** * cloud.aws.ec2.secret_key: AWS Secret key specific for EC2 API calls. Defaults to cloud.aws.secret_key. * @see AwsEc2Service#SECRET_SETTING */ - Setting SECRET_SETTING = new Setting<>("cloud.aws.ec2.secret_key", AwsEc2Service.SECRET_SETTING, Function.identity(), - Property.NodeScope, Property.Filtered); + Setting SECRET_SETTING = new Setting<>("cloud.aws.ec2.secret_key", AwsEc2Service.SECRET_SETTING, + SecureString::new, Property.NodeScope, Property.Filtered, Property.Deprecated); /** * cloud.aws.ec2.protocol: Protocol for AWS API specific for EC2 API calls: http or https. Defaults to cloud.aws.protocol. * @see AwsEc2Service#PROTOCOL_SETTING */ Setting PROTOCOL_SETTING = new Setting<>("cloud.aws.ec2.protocol", AwsEc2Service.PROTOCOL_SETTING, - s -> Protocol.valueOf(s.toUpperCase(Locale.ROOT)), Property.NodeScope); + s -> Protocol.valueOf(s.toUpperCase(Locale.ROOT)), Property.NodeScope, Property.Deprecated); /** * cloud.aws.ec2.proxy.host: In case of proxy, define its hostname/IP specific for EC2 API calls. Defaults to cloud.aws.proxy.host. * @see AwsEc2Service#PROXY_HOST_SETTING */ Setting PROXY_HOST_SETTING = new Setting<>("cloud.aws.ec2.proxy.host", AwsEc2Service.PROXY_HOST_SETTING, - Function.identity(), Property.NodeScope); + Function.identity(), Property.NodeScope, Property.Deprecated); /** * cloud.aws.ec2.proxy.port: In case of proxy, define its port specific for EC2 API calls. Defaults to cloud.aws.proxy.port. * @see AwsEc2Service#PROXY_PORT_SETTING */ Setting PROXY_PORT_SETTING = new Setting<>("cloud.aws.ec2.proxy.port", AwsEc2Service.PROXY_PORT_SETTING, - s -> Setting.parseInt(s, 0, 1<<16, "cloud.aws.ec2.proxy.port"), Property.NodeScope); + s -> Setting.parseInt(s, 0, 1<<16, "cloud.aws.ec2.proxy.port"), Property.NodeScope, Property.Deprecated); /** * cloud.aws.ec2.proxy.username: In case of proxy with auth, define the username specific for EC2 API calls. * Defaults to cloud.aws.proxy.username. * @see AwsEc2Service#PROXY_USERNAME_SETTING */ - Setting PROXY_USERNAME_SETTING = new Setting<>("cloud.aws.ec2.proxy.username", AwsEc2Service.PROXY_USERNAME_SETTING, - Function.identity(), Property.NodeScope); + Setting PROXY_USERNAME_SETTING = new Setting<>("cloud.aws.ec2.proxy.username", AwsEc2Service.PROXY_USERNAME_SETTING, + SecureString::new, Property.NodeScope, Property.Filtered, Property.Deprecated); /** * cloud.aws.ec2.proxy.password: In case of proxy with auth, define the password specific for EC2 API calls. * Defaults to cloud.aws.proxy.password. * @see AwsEc2Service#PROXY_PASSWORD_SETTING */ - Setting PROXY_PASSWORD_SETTING = new Setting<>("cloud.aws.ec2.proxy.password", AwsEc2Service.PROXY_PASSWORD_SETTING, - Function.identity(), Property.NodeScope, Property.Filtered); + Setting PROXY_PASSWORD_SETTING = new Setting<>("cloud.aws.ec2.proxy.password", AwsEc2Service.PROXY_PASSWORD_SETTING, + SecureString::new, Property.NodeScope, Property.Filtered, Property.Deprecated); /** * cloud.aws.ec2.signer: If you are using an old AWS API version, you can define a Signer. Specific for EC2 API calls. * Defaults to cloud.aws.signer. * @see AwsEc2Service#SIGNER_SETTING */ Setting SIGNER_SETTING = new Setting<>("cloud.aws.ec2.signer", AwsEc2Service.SIGNER_SETTING, Function.identity(), - Property.NodeScope); + Property.NodeScope, Property.Deprecated); /** * cloud.aws.ec2.region: Region specific for EC2 API calls. Defaults to cloud.aws.region. * @see AwsEc2Service#REGION_SETTING */ Setting REGION_SETTING = new Setting<>("cloud.aws.ec2.region", AwsEc2Service.REGION_SETTING, - s -> s.toLowerCase(Locale.ROOT), Property.NodeScope); + s -> s.toLowerCase(Locale.ROOT), Property.NodeScope, Property.Deprecated); /** * cloud.aws.ec2.endpoint: Endpoint. If not set, endpoint will be guessed based on region setting. */ - Setting ENDPOINT_SETTING = Setting.simpleString("cloud.aws.ec2.endpoint", Property.NodeScope); + Setting ENDPOINT_SETTING = Setting.simpleString("cloud.aws.ec2.endpoint", Property.NodeScope, Property.Deprecated); /** * cloud.aws.ec2.read_timeout: Socket read timeout. Defaults to cloud.aws.read_timeout * @see AwsEc2Service#READ_TIMEOUT */ Setting READ_TIMEOUT = - Setting.timeSetting("cloud.aws.ec2.read_timeout", AwsEc2Service.READ_TIMEOUT, Property.NodeScope); + Setting.timeSetting("cloud.aws.ec2.read_timeout", AwsEc2Service.READ_TIMEOUT, Property.NodeScope, Property.Deprecated); } /** @@ -172,6 +177,40 @@ interface AwsEc2Service { public static final String TAG_PREFIX = "tag:"; } + /** The access key (ie login id) for connecting to ec2. */ + Setting ACCESS_KEY_SETTING = SecureSetting.secureString("discovery.ec2.access_key", CLOUD_EC2.KEY_SETTING, false); + + /** The secret key (ie password) for connecting to ec2. */ + Setting SECRET_KEY_SETTING = SecureSetting.secureString("discovery.ec2.secret_key", CLOUD_EC2.SECRET_SETTING, false); + + /** An override for the ec2 endpoint to connect to. */ + Setting ENDPOINT_SETTING = new Setting<>("discovery.ec2.endpoint", CLOUD_EC2.ENDPOINT_SETTING, + s -> s.toLowerCase(Locale.ROOT), Setting.Property.NodeScope); + + /** The protocol to use to connect to to ec2. */ + Setting PROTOCOL_SETTING = new Setting<>("discovery.ec2.protocol", CLOUD_EC2.PROTOCOL_SETTING, + s -> Protocol.valueOf(s.toUpperCase(Locale.ROOT)), Setting.Property.NodeScope); + + /** The host name of a proxy to connect to ec2 through. */ + Setting PROXY_HOST_SETTING = new Setting<>("discovery.ec2.proxy.host", CLOUD_EC2.PROXY_HOST_SETTING, + Function.identity(), Setting.Property.NodeScope); + + /** The port of a proxy to connect to ec2 through. */ + Setting PROXY_PORT_SETTING = Setting.intSetting("discovery.ec2.proxy.port", CLOUD_EC2.PROXY_PORT_SETTING, + 0, Setting.Property.NodeScope); + + /** The username of a proxy to connect to s3 through. */ + Setting PROXY_USERNAME_SETTING = SecureSetting.secureString("discovery.ec2.proxy.username", + CLOUD_EC2.PROXY_USERNAME_SETTING, false); + + /** The password of a proxy to connect to s3 through. */ + Setting PROXY_PASSWORD_SETTING = SecureSetting.secureString("discovery.ec2.proxy.password", + CLOUD_EC2.PROXY_PASSWORD_SETTING, false); + + /** The socket timeout for connecting to s3. */ + Setting READ_TIMEOUT_SETTING = Setting.timeSetting("discovery.ec2.read_timeout", + CLOUD_EC2.READ_TIMEOUT, Setting.Property.NodeScope); + /** * discovery.ec2.host_type: The type of host type to use to communicate with other instances. * Can be one of private_ip, public_ip, private_dns, public_dns or tag:XXXX where diff --git a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java index 6f4252c447a..8131b5be9ba 100644 --- a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java +++ b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java @@ -38,6 +38,7 @@ import org.apache.logging.log4j.Logger; import org.elasticsearch.common.Randomness; import org.elasticsearch.common.Strings; import org.elasticsearch.common.component.AbstractComponent; +import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Settings; class AwsEc2ServiceImpl extends AbstractComponent implements AwsEc2Service, Closeable { @@ -68,14 +69,15 @@ class AwsEc2ServiceImpl extends AbstractComponent implements AwsEc2Service, Clos protected static AWSCredentialsProvider buildCredentials(Logger logger, Settings settings) { AWSCredentialsProvider credentials; - String key = CLOUD_EC2.KEY_SETTING.get(settings); - String secret = CLOUD_EC2.SECRET_SETTING.get(settings); - if (key.isEmpty() && secret.isEmpty()) { - logger.debug("Using either environment variables, system properties or instance profile credentials"); - credentials = new DefaultAWSCredentialsProviderChain(); - } else { - logger.debug("Using basic key/secret credentials"); - credentials = new StaticCredentialsProvider(new BasicAWSCredentials(key, secret)); + try (SecureString key = DISCOVERY_EC2.ACCESS_KEY_SETTING.get(settings); + SecureString secret = DISCOVERY_EC2.SECRET_KEY_SETTING.get(settings)) { + if (key.length() == 0 && secret.length() == 0) { + logger.debug("Using either environment variables, system properties or instance profile credentials"); + credentials = new DefaultAWSCredentialsProviderChain(); + } else { + logger.debug("Using basic key/secret credentials"); + credentials = new StaticCredentialsProvider(new BasicAWSCredentials(key.toString(), secret.toString())); + } } return credentials; @@ -86,19 +88,20 @@ class AwsEc2ServiceImpl extends AbstractComponent implements AwsEc2Service, Clos // the response metadata cache is only there for diagnostics purposes, // but can force objects from every response to the old generation. clientConfiguration.setResponseMetadataCacheSize(0); - clientConfiguration.setProtocol(CLOUD_EC2.PROTOCOL_SETTING.get(settings)); + clientConfiguration.setProtocol(DISCOVERY_EC2.PROTOCOL_SETTING.get(settings)); - if (PROXY_HOST_SETTING.exists(settings) || CLOUD_EC2.PROXY_HOST_SETTING.exists(settings)) { - String proxyHost = CLOUD_EC2.PROXY_HOST_SETTING.get(settings); - Integer proxyPort = CLOUD_EC2.PROXY_PORT_SETTING.get(settings); - String proxyUsername = CLOUD_EC2.PROXY_USERNAME_SETTING.get(settings); - String proxyPassword = CLOUD_EC2.PROXY_PASSWORD_SETTING.get(settings); + if (PROXY_HOST_SETTING.exists(settings) || DISCOVERY_EC2.PROXY_HOST_SETTING.exists(settings)) { + String proxyHost = DISCOVERY_EC2.PROXY_HOST_SETTING.get(settings); + Integer proxyPort = DISCOVERY_EC2.PROXY_PORT_SETTING.get(settings); + try (SecureString proxyUsername = DISCOVERY_EC2.PROXY_USERNAME_SETTING.get(settings); + SecureString proxyPassword = DISCOVERY_EC2.PROXY_PASSWORD_SETTING.get(settings)) { - clientConfiguration - .withProxyHost(proxyHost) - .withProxyPort(proxyPort) - .withProxyUsername(proxyUsername) - .withProxyPassword(proxyPassword); + clientConfiguration + .withProxyHost(proxyHost) + .withProxyPort(proxyPort) + .withProxyUsername(proxyUsername.toString()) + .withProxyPassword(proxyPassword.toString()); + } } // #155: we might have 3rd party users using older EC2 API version @@ -125,15 +128,15 @@ class AwsEc2ServiceImpl extends AbstractComponent implements AwsEc2Service, Clos 10, false); clientConfiguration.setRetryPolicy(retryPolicy); - clientConfiguration.setSocketTimeout((int) CLOUD_EC2.READ_TIMEOUT.get(settings).millis()); + clientConfiguration.setSocketTimeout((int) DISCOVERY_EC2.READ_TIMEOUT_SETTING.get(settings).millis()); return clientConfiguration; } protected static String findEndpoint(Logger logger, Settings settings) { String endpoint = null; - if (CLOUD_EC2.ENDPOINT_SETTING.exists(settings)) { - endpoint = CLOUD_EC2.ENDPOINT_SETTING.get(settings); + if (DISCOVERY_EC2.ENDPOINT_SETTING.exists(settings) || CLOUD_EC2.ENDPOINT_SETTING.exists(settings)) { + endpoint = DISCOVERY_EC2.ENDPOINT_SETTING.get(settings); logger.debug("using explicit ec2 endpoint [{}]", endpoint); } else if (REGION_SETTING.exists(settings) || CLOUD_EC2.REGION_SETTING.exists(settings)) { final String region = CLOUD_EC2.REGION_SETTING.get(settings); diff --git a/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImplTests.java b/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImplTests.java index 51a14d9cf60..0e82d50c17c 100644 --- a/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImplTests.java +++ b/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImplTests.java @@ -24,6 +24,8 @@ import com.amazonaws.Protocol; import com.amazonaws.auth.AWSCredentials; import com.amazonaws.auth.AWSCredentialsProvider; import com.amazonaws.auth.DefaultAWSCredentialsProviderChain; +import org.elasticsearch.common.settings.MockSecureSettings; +import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.discovery.ec2.AwsEc2Service; import org.elasticsearch.discovery.ec2.AwsEc2ServiceImpl; @@ -42,19 +44,35 @@ public class AwsEc2ServiceImplTests extends ESTestCase { } public void testAWSCredentialsWithElasticsearchAwsSettings() { + MockSecureSettings secureSettings = new MockSecureSettings(); + secureSettings.setString("discovery.ec2.access_key", "aws_key"); + secureSettings.setString("discovery.ec2.secret_key", "aws_secret"); + Settings settings = Settings.builder().setSecureSettings(secureSettings).build(); + launchAWSCredentialsWithElasticsearchSettingsTest(settings, "aws_key", "aws_secret"); + } + + public void testAWSCredentialsWithElasticsearchAwsSettingsBackcompat() { Settings settings = Settings.builder() .put(AwsEc2Service.KEY_SETTING.getKey(), "aws_key") .put(AwsEc2Service.SECRET_SETTING.getKey(), "aws_secret") .build(); launchAWSCredentialsWithElasticsearchSettingsTest(settings, "aws_key", "aws_secret"); + assertSettingDeprecationsAndWarnings(new Setting[] { + AwsEc2Service.KEY_SETTING, + AwsEc2Service.SECRET_SETTING + }); } - public void testAWSCredentialsWithElasticsearchEc2Settings() { + public void testAWSCredentialsWithElasticsearchEc2SettingsBackcompat() { Settings settings = Settings.builder() .put(AwsEc2Service.CLOUD_EC2.KEY_SETTING.getKey(), "ec2_key") .put(AwsEc2Service.CLOUD_EC2.SECRET_SETTING.getKey(), "ec2_secret") .build(); launchAWSCredentialsWithElasticsearchSettingsTest(settings, "ec2_key", "ec2_secret"); + assertSettingDeprecationsAndWarnings(new Setting[] { + AwsEc2Service.CLOUD_EC2.KEY_SETTING, + AwsEc2Service.CLOUD_EC2.SECRET_SETTING + }); } public void testAWSCredentialsWithElasticsearchAwsAndEc2Settings() { @@ -65,6 +83,12 @@ public class AwsEc2ServiceImplTests extends ESTestCase { .put(AwsEc2Service.CLOUD_EC2.SECRET_SETTING.getKey(), "ec2_secret") .build(); launchAWSCredentialsWithElasticsearchSettingsTest(settings, "ec2_key", "ec2_secret"); + assertSettingDeprecationsAndWarnings(new Setting[] { + AwsEc2Service.KEY_SETTING, + AwsEc2Service.SECRET_SETTING, + AwsEc2Service.CLOUD_EC2.KEY_SETTING, + AwsEc2Service.CLOUD_EC2.SECRET_SETTING + }); } protected void launchAWSCredentialsWithElasticsearchSettingsTest(Settings settings, String expectedKey, String expectedSecret) { @@ -79,6 +103,21 @@ public class AwsEc2ServiceImplTests extends ESTestCase { } public void testAWSConfigurationWithAwsSettings() { + MockSecureSettings secureSettings = new MockSecureSettings(); + secureSettings.setString("discovery.ec2.proxy.username", "aws_proxy_username"); + secureSettings.setString("discovery.ec2.proxy.password", "aws_proxy_password"); + Settings settings = Settings.builder() + .put("discovery.ec2.protocol", "http") + .put("discovery.ec2.proxy.host", "aws_proxy_host") + .put("discovery.ec2.proxy.port", 8080) + .put("discovery.ec2.read_timeout", "10s") + .setSecureSettings(secureSettings) + .build(); + launchAWSConfigurationTest(settings, Protocol.HTTP, "aws_proxy_host", 8080, "aws_proxy_username", "aws_proxy_password", + null, 10000); + } + + public void testAWSConfigurationWithAwsSettingsBackcompat() { Settings settings = Settings.builder() .put(AwsEc2Service.PROTOCOL_SETTING.getKey(), "http") .put(AwsEc2Service.PROXY_HOST_SETTING.getKey(), "aws_proxy_host") @@ -90,6 +129,15 @@ public class AwsEc2ServiceImplTests extends ESTestCase { .build(); launchAWSConfigurationTest(settings, Protocol.HTTP, "aws_proxy_host", 8080, "aws_proxy_username", "aws_proxy_password", "AWS3SignerType", 10000); + assertSettingDeprecationsAndWarnings(new Setting[] { + AwsEc2Service.PROTOCOL_SETTING, + AwsEc2Service.PROXY_HOST_SETTING, + AwsEc2Service.PROXY_PORT_SETTING, + AwsEc2Service.PROXY_USERNAME_SETTING, + AwsEc2Service.PROXY_PASSWORD_SETTING, + AwsEc2Service.SIGNER_SETTING, + AwsEc2Service.READ_TIMEOUT + }); } public void testAWSConfigurationWithAwsAndEc2Settings() { @@ -100,6 +148,7 @@ public class AwsEc2ServiceImplTests extends ESTestCase { .put(AwsEc2Service.PROXY_USERNAME_SETTING.getKey(), "aws_proxy_username") .put(AwsEc2Service.PROXY_PASSWORD_SETTING.getKey(), "aws_proxy_password") .put(AwsEc2Service.SIGNER_SETTING.getKey(), "AWS3SignerType") + .put(AwsEc2Service.READ_TIMEOUT.getKey(), "20s") .put(AwsEc2Service.CLOUD_EC2.PROTOCOL_SETTING.getKey(), "https") .put(AwsEc2Service.CLOUD_EC2.PROXY_HOST_SETTING.getKey(), "ec2_proxy_host") .put(AwsEc2Service.CLOUD_EC2.PROXY_PORT_SETTING.getKey(), 8081) @@ -110,6 +159,22 @@ public class AwsEc2ServiceImplTests extends ESTestCase { .build(); launchAWSConfigurationTest(settings, Protocol.HTTPS, "ec2_proxy_host", 8081, "ec2_proxy_username", "ec2_proxy_password", "NoOpSignerType", 10000); + assertSettingDeprecationsAndWarnings(new Setting[] { + AwsEc2Service.PROTOCOL_SETTING, + AwsEc2Service.PROXY_HOST_SETTING, + AwsEc2Service.PROXY_PORT_SETTING, + AwsEc2Service.PROXY_USERNAME_SETTING, + AwsEc2Service.PROXY_PASSWORD_SETTING, + AwsEc2Service.SIGNER_SETTING, + AwsEc2Service.READ_TIMEOUT, + AwsEc2Service.CLOUD_EC2.PROTOCOL_SETTING, + AwsEc2Service.CLOUD_EC2.PROXY_HOST_SETTING, + AwsEc2Service.CLOUD_EC2.PROXY_PORT_SETTING, + AwsEc2Service.CLOUD_EC2.PROXY_USERNAME_SETTING, + AwsEc2Service.CLOUD_EC2.PROXY_PASSWORD_SETTING, + AwsEc2Service.CLOUD_EC2.SIGNER_SETTING, + AwsEc2Service.CLOUD_EC2.READ_TIMEOUT + }); } protected void launchAWSConfigurationTest(Settings settings, @@ -138,11 +203,22 @@ public class AwsEc2ServiceImplTests extends ESTestCase { } public void testSpecificEndpoint() { + Settings settings = Settings.builder() + .put(AwsEc2Service.DISCOVERY_EC2.ENDPOINT_SETTING.getKey(), "ec2.endpoint") + .build(); + String endpoint = AwsEc2ServiceImpl.findEndpoint(logger, settings); + assertThat(endpoint, is("ec2.endpoint")); + } + + public void testSpecificEndpointBackcompat() { Settings settings = Settings.builder() .put(AwsEc2Service.CLOUD_EC2.ENDPOINT_SETTING.getKey(), "ec2.endpoint") .build(); String endpoint = AwsEc2ServiceImpl.findEndpoint(logger, settings); assertThat(endpoint, is("ec2.endpoint")); + assertSettingDeprecationsAndWarnings(new Setting[] { + AwsEc2Service.CLOUD_EC2.ENDPOINT_SETTING + }); } public void testRegionWithAwsSettings() { @@ -151,6 +227,9 @@ public class AwsEc2ServiceImplTests extends ESTestCase { .build(); String endpoint = AwsEc2ServiceImpl.findEndpoint(logger, settings); assertThat(endpoint, is("ec2.eu-west-1.amazonaws.com")); + assertSettingDeprecationsAndWarnings(new Setting[] { + AwsEc2Service.REGION_SETTING + }); } public void testRegionWithAwsAndEc2Settings() { @@ -160,6 +239,10 @@ public class AwsEc2ServiceImplTests extends ESTestCase { .build(); String endpoint = AwsEc2ServiceImpl.findEndpoint(logger, settings); assertThat(endpoint, is("ec2.us-west-1.amazonaws.com")); + assertSettingDeprecationsAndWarnings(new Setting[] { + AwsEc2Service.REGION_SETTING, + AwsEc2Service.CLOUD_EC2.REGION_SETTING + }); } public void testInvalidRegion() { @@ -170,5 +253,8 @@ public class AwsEc2ServiceImplTests extends ESTestCase { AwsEc2ServiceImpl.findEndpoint(logger, settings); }); assertThat(e.getMessage(), containsString("No automatic endpoint could be derived from region")); + assertSettingDeprecationsAndWarnings(new Setting[] { + AwsEc2Service.REGION_SETTING + }); } } diff --git a/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/Ec2DiscoverySettingsTests.java b/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/Ec2DiscoverySettingsTests.java index a72b973ea1f..d1ac8cfa0cf 100644 --- a/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/Ec2DiscoverySettingsTests.java +++ b/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/Ec2DiscoverySettingsTests.java @@ -20,6 +20,7 @@ package org.elasticsearch.discovery.ec2; import com.amazonaws.Protocol; +import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.test.ESTestCase; @@ -68,6 +69,17 @@ public class Ec2DiscoverySettingsTests extends ESTestCase { assertThat(AwsEc2Service.CLOUD_EC2.SIGNER_SETTING.get(nodeSettings), is("global-signer")); assertThat(AwsEc2Service.CLOUD_EC2.REGION_SETTING.get(nodeSettings), is("global-region")); assertThat(AwsEc2Service.CLOUD_EC2.ENDPOINT_SETTING.get(nodeSettings), isEmptyString()); + assertSettingDeprecationsAndWarnings(new Setting[] { + AwsEc2Service.KEY_SETTING, + AwsEc2Service.SECRET_SETTING, + AwsEc2Service.PROTOCOL_SETTING, + AwsEc2Service.PROXY_HOST_SETTING, + AwsEc2Service.PROXY_PORT_SETTING, + AwsEc2Service.PROXY_USERNAME_SETTING, + AwsEc2Service.PROXY_PASSWORD_SETTING, + AwsEc2Service.SIGNER_SETTING, + AwsEc2Service.REGION_SETTING + }); } /** @@ -85,6 +97,27 @@ public class Ec2DiscoverySettingsTests extends ESTestCase { assertThat(AwsEc2Service.CLOUD_EC2.SIGNER_SETTING.get(nodeSettings), is("ec2-signer")); assertThat(AwsEc2Service.CLOUD_EC2.REGION_SETTING.get(nodeSettings), is("ec2-region")); assertThat(AwsEc2Service.CLOUD_EC2.ENDPOINT_SETTING.get(nodeSettings), is("ec2-endpoint")); + assertSettingDeprecationsAndWarnings(new Setting[] { + AwsEc2Service.KEY_SETTING, + AwsEc2Service.SECRET_SETTING, + AwsEc2Service.PROTOCOL_SETTING, + AwsEc2Service.PROXY_HOST_SETTING, + AwsEc2Service.PROXY_PORT_SETTING, + AwsEc2Service.PROXY_USERNAME_SETTING, + AwsEc2Service.PROXY_PASSWORD_SETTING, + AwsEc2Service.SIGNER_SETTING, + AwsEc2Service.REGION_SETTING, + AwsEc2Service.CLOUD_EC2.KEY_SETTING, + AwsEc2Service.CLOUD_EC2.SECRET_SETTING, + AwsEc2Service.CLOUD_EC2.PROTOCOL_SETTING, + AwsEc2Service.CLOUD_EC2.PROXY_HOST_SETTING, + AwsEc2Service.CLOUD_EC2.PROXY_PORT_SETTING, + AwsEc2Service.CLOUD_EC2.PROXY_USERNAME_SETTING, + AwsEc2Service.CLOUD_EC2.PROXY_PASSWORD_SETTING, + AwsEc2Service.CLOUD_EC2.SIGNER_SETTING, + AwsEc2Service.CLOUD_EC2.REGION_SETTING, + AwsEc2Service.CLOUD_EC2.ENDPOINT_SETTING + }); } private Settings buildSettings(Settings... global) { diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java index 377f73e5c79..146e62cc617 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java @@ -331,7 +331,7 @@ public abstract class ESTestCase extends LuceneTestCase { final Set actualWarningValues = actualWarnings.stream().map(DeprecationLogger::extractWarningValueFromWarningHeader).collect(Collectors.toSet()); for (String msg : expectedWarnings) { - assertTrue(actualWarningValues.contains(DeprecationLogger.escape(msg))); + assertTrue(msg, actualWarningValues.contains(DeprecationLogger.escape(msg))); } assertEquals("Expected " + expectedWarnings.length + " warnings but found " + actualWarnings.size() + "\nExpected: " + Arrays.asList(expectedWarnings) + "\nActual: " + actualWarnings, From 6e0b445abbcd9e717b9f8683203a6917fb6ad7bd Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 7 Apr 2017 14:07:59 -0700 Subject: [PATCH 07/26] Add registration of new discovery settings This was forgotten as part of #23961 --- .../elasticsearch/discovery/ec2/Ec2DiscoveryPlugin.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/Ec2DiscoveryPlugin.java b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/Ec2DiscoveryPlugin.java index d78197e875b..5c091b825a4 100644 --- a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/Ec2DiscoveryPlugin.java +++ b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/Ec2DiscoveryPlugin.java @@ -143,6 +143,15 @@ public class Ec2DiscoveryPlugin extends Plugin implements DiscoveryPlugin, Close AwsEc2Service.CLOUD_EC2.ENDPOINT_SETTING, AwsEc2Service.CLOUD_EC2.READ_TIMEOUT, // Register EC2 discovery settings: discovery.ec2 + AwsEc2Service.DISCOVERY_EC2.ACCESS_KEY_SETTING, + AwsEc2Service.DISCOVERY_EC2.SECRET_KEY_SETTING, + AwsEc2Service.DISCOVERY_EC2.ENDPOINT_SETTING, + AwsEc2Service.DISCOVERY_EC2.PROTOCOL_SETTING, + AwsEc2Service.DISCOVERY_EC2.PROXY_HOST_SETTING, + AwsEc2Service.DISCOVERY_EC2.PROXY_PORT_SETTING, + AwsEc2Service.DISCOVERY_EC2.PROXY_USERNAME_SETTING, + AwsEc2Service.DISCOVERY_EC2.PROXY_PASSWORD_SETTING, + AwsEc2Service.DISCOVERY_EC2.READ_TIMEOUT_SETTING, AwsEc2Service.DISCOVERY_EC2.HOST_TYPE_SETTING, AwsEc2Service.DISCOVERY_EC2.ANY_GROUP_SETTING, AwsEc2Service.DISCOVERY_EC2.GROUPS_SETTING, From 73b8aad9a377a396c2323fa146ea6bd03162578a Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 7 Apr 2017 14:18:06 -0700 Subject: [PATCH 08/26] Settings: Disallow secure setting to exist in normal settings (#23976) This commit removes the "legacy" feature of secure settings, which setup a parallel setting that was a fallback in the insecure elasticsearch.yml. This was previously used to allow the new secure setting name to be that of the old setting name, but is now not in use due to other refactorings. It is much cleaner to just have all secure settings use new setting names. If in the future we want to reuse the previous setting name, once support for the insecure settings have been removed, we can then rename the secure setting. This also adds a test for the behavior. --- .../common/settings/SecureSetting.java | 28 ++++--------------- .../common/settings/ScopedSettingsTests.java | 4 +-- .../common/settings/SettingsTests.java | 7 +++++ .../InternalSettingsPreparerTests.java | 2 +- .../discovery/ec2/AwsEc2Service.java | 8 +++--- .../repositories/s3/S3Repository.java | 8 +++--- 6 files changed, 23 insertions(+), 34 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/settings/SecureSetting.java b/core/src/main/java/org/elasticsearch/common/settings/SecureSetting.java index 16757187196..a9e4effb0d9 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/SecureSetting.java +++ b/core/src/main/java/org/elasticsearch/common/settings/SecureSetting.java @@ -87,6 +87,10 @@ public abstract class SecureSetting extends Setting { checkDeprecation(settings); final SecureSettings secureSettings = settings.getSecureSettings(); if (secureSettings == null || secureSettings.getSettingNames().contains(getKey()) == false) { + if (super.exists(settings)) { + throw new IllegalArgumentException("Setting [" + getKey() + "] is a secure setting" + + " and must be stored inside the Elasticsearch keystore, but was found inside elasticsearch.yml"); + } return getFallback(settings); } try { @@ -117,14 +121,7 @@ public abstract class SecureSetting extends Setting { * This may be any sensitive string, e.g. a username, a password, an auth token, etc. */ public static Setting secureString(String name, Setting fallback, - boolean allowLegacy, Property... properties) { - final Setting legacy; - if (allowLegacy) { - Property[] legacyProperties = ArrayUtils.concat(properties, LEGACY_PROPERTIES, Property.class); - legacy = Setting.simpleString(name, legacyProperties); - } else { - legacy = null; - } + Property... properties) { return new SecureSetting(name, properties) { @Override protected SecureString getSecret(SecureSettings secureSettings) throws GeneralSecurityException { @@ -132,26 +129,11 @@ public abstract class SecureSetting extends Setting { } @Override SecureString getFallback(Settings settings) { - if (legacy != null && legacy.exists(settings)) { - return new SecureString(legacy.get(settings).toCharArray()); - } if (fallback != null) { return fallback.get(settings); } return new SecureString(new char[0]); // this means "setting does not exist" } - @Override - protected void checkDeprecation(Settings settings) { - super.checkDeprecation(settings); - if (legacy != null) { - legacy.checkDeprecation(settings); - } - } - @Override - public boolean exists(Settings settings) { - // handle legacy, which is internal to this setting - return super.exists(settings) || legacy != null && legacy.exists(settings); - } }; } diff --git a/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java index 76905d43799..01ace21ad12 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java @@ -454,7 +454,7 @@ public class ScopedSettingsTests extends ESTestCase { assertThat(e.getMessage(), startsWith("unknown secure setting [some.secure.setting]")); ClusterSettings clusterSettings2 = new ClusterSettings(settings, - Collections.singleton(SecureSetting.secureString("some.secure.setting", null, false))); + Collections.singleton(SecureSetting.secureString("some.secure.setting", null))); clusterSettings2.validate(settings); } @@ -463,7 +463,7 @@ public class ScopedSettingsTests extends ESTestCase { secureSettings.setString("some.secure.setting", "secret"); Settings settings = Settings.builder().setSecureSettings(secureSettings).build(); ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, - Collections.singleton(SecureSetting.secureString("some.secure.setting", null, false))); + Collections.singleton(SecureSetting.secureString("some.secure.setting", null))); Settings diffed = clusterSettings.diff(Settings.EMPTY, settings); assertTrue(diffed.isEmpty()); diff --git a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java b/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java index f747a20e468..c1dc07116ec 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java @@ -555,4 +555,11 @@ public class SettingsTests extends ESTestCase { MockSecureSettings secureSettings = new MockSecureSettings(); assertTrue(Settings.builder().setSecureSettings(secureSettings).build().isEmpty()); } + + public void testSecureSettingConflict() { + Setting setting = SecureSetting.secureString("something.secure", null); + Settings settings = Settings.builder().put("something.secure", "notreallysecure").build(); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> setting.get(settings)); + assertTrue(e.getMessage().contains("must be stored inside the Elasticsearch keystore")); + } } diff --git a/core/src/test/java/org/elasticsearch/node/internal/InternalSettingsPreparerTests.java b/core/src/test/java/org/elasticsearch/node/internal/InternalSettingsPreparerTests.java index 21c1811616d..daaeab80143 100644 --- a/core/src/test/java/org/elasticsearch/node/internal/InternalSettingsPreparerTests.java +++ b/core/src/test/java/org/elasticsearch/node/internal/InternalSettingsPreparerTests.java @@ -178,7 +178,7 @@ public class InternalSettingsPreparerTests extends ESTestCase { secureSettings.setString("foo", "secret"); Settings input = Settings.builder().put(baseEnvSettings).setSecureSettings(secureSettings).build(); Environment env = InternalSettingsPreparer.prepareEnvironment(input, null); - Setting fakeSetting = SecureSetting.secureString("foo", null, false); + Setting fakeSetting = SecureSetting.secureString("foo", null); assertEquals("secret", fakeSetting.get(env.settings()).toString()); } diff --git a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2Service.java b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2Service.java index a1ca208e4ac..0b1fdca257d 100644 --- a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2Service.java +++ b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2Service.java @@ -178,10 +178,10 @@ interface AwsEc2Service { } /** The access key (ie login id) for connecting to ec2. */ - Setting ACCESS_KEY_SETTING = SecureSetting.secureString("discovery.ec2.access_key", CLOUD_EC2.KEY_SETTING, false); + Setting ACCESS_KEY_SETTING = SecureSetting.secureString("discovery.ec2.access_key", CLOUD_EC2.KEY_SETTING); /** The secret key (ie password) for connecting to ec2. */ - Setting SECRET_KEY_SETTING = SecureSetting.secureString("discovery.ec2.secret_key", CLOUD_EC2.SECRET_SETTING, false); + Setting SECRET_KEY_SETTING = SecureSetting.secureString("discovery.ec2.secret_key", CLOUD_EC2.SECRET_SETTING); /** An override for the ec2 endpoint to connect to. */ Setting ENDPOINT_SETTING = new Setting<>("discovery.ec2.endpoint", CLOUD_EC2.ENDPOINT_SETTING, @@ -201,11 +201,11 @@ interface AwsEc2Service { /** The username of a proxy to connect to s3 through. */ Setting PROXY_USERNAME_SETTING = SecureSetting.secureString("discovery.ec2.proxy.username", - CLOUD_EC2.PROXY_USERNAME_SETTING, false); + CLOUD_EC2.PROXY_USERNAME_SETTING); /** The password of a proxy to connect to s3 through. */ Setting PROXY_PASSWORD_SETTING = SecureSetting.secureString("discovery.ec2.proxy.password", - CLOUD_EC2.PROXY_PASSWORD_SETTING, false); + CLOUD_EC2.PROXY_PASSWORD_SETTING); /** The socket timeout for connecting to s3. */ Setting READ_TIMEOUT_SETTING = Setting.timeSetting("discovery.ec2.read_timeout", diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java index 3ed32a7b8f3..50e9b998ad6 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java @@ -65,11 +65,11 @@ class S3Repository extends BlobStoreRepository { /** The access key (ie login id) for connecting to s3. */ public static final AffixSetting ACCESS_KEY_SETTING = Setting.affixKeySetting(PREFIX, "access_key", - key -> SecureSetting.secureString(key, Repositories.KEY_SETTING, false)); + key -> SecureSetting.secureString(key, Repositories.KEY_SETTING)); /** The secret key (ie password) for connecting to s3. */ public static final AffixSetting SECRET_KEY_SETTING = Setting.affixKeySetting(PREFIX, "secret_key", - key -> SecureSetting.secureString(key, Repositories.SECRET_SETTING, false)); + key -> SecureSetting.secureString(key, Repositories.SECRET_SETTING)); /** An override for the s3 endpoint to connect to. */ public static final AffixSetting ENDPOINT_SETTING = Setting.affixKeySetting(PREFIX, "endpoint", @@ -89,11 +89,11 @@ class S3Repository extends BlobStoreRepository { /** The username of a proxy to connect to s3 through. */ public static final AffixSetting PROXY_USERNAME_SETTING = Setting.affixKeySetting(PREFIX, "proxy.username", - key -> SecureSetting.secureString(key, AwsS3Service.PROXY_USERNAME_SETTING, false)); + key -> SecureSetting.secureString(key, AwsS3Service.PROXY_USERNAME_SETTING)); /** The password of a proxy to connect to s3 through. */ public static final AffixSetting PROXY_PASSWORD_SETTING = Setting.affixKeySetting(PREFIX, "proxy.password", - key -> SecureSetting.secureString(key, AwsS3Service.PROXY_PASSWORD_SETTING, false)); + key -> SecureSetting.secureString(key, AwsS3Service.PROXY_PASSWORD_SETTING)); /** The socket timeout for connecting to s3. */ public static final AffixSetting READ_TIMEOUT_SETTING = Setting.affixKeySetting(PREFIX, "read_timeout", From 05e2ea1aefa5e5e66ecaf7c3a308ae23ff3fb117 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 7 Apr 2017 16:46:17 -0700 Subject: [PATCH 09/26] AWS Plugins: Remove signer type setting (#23984) This commit removes support for s3 signer type in 6.0, and adds a note to the migration guide. closes #22599 --- .../migration/migrate_6_0/plugins.asciidoc | 6 ++++++ .../discovery/ec2/AwsEc2Service.java | 11 ---------- .../discovery/ec2/AwsEc2ServiceImpl.java | 7 ------- .../discovery/ec2/Ec2DiscoveryPlugin.java | 2 -- .../discovery/ec2/AwsEc2ServiceImplTests.java | 18 ++++------------- .../ec2/Ec2DiscoverySettingsTests.java | 7 ------- .../repositories/s3/AwsS3Service.java | 13 ------------ .../repositories/s3/InternalAwsS3Service.java | 7 ------- .../repositories/s3/S3RepositoryPlugin.java | 2 -- .../s3/AwsS3ServiceImplTests.java | 20 ++++++------------- 10 files changed, 16 insertions(+), 77 deletions(-) diff --git a/docs/reference/migration/migrate_6_0/plugins.asciidoc b/docs/reference/migration/migrate_6_0/plugins.asciidoc index d2032d683b5..e5861887cda 100644 --- a/docs/reference/migration/migrate_6_0/plugins.asciidoc +++ b/docs/reference/migration/migrate_6_0/plugins.asciidoc @@ -21,6 +21,8 @@ region inside the repository settings. Instead, specify the full endpoint if a c s3 location is needed, or rely on the default behavior which automatically locates the region of the configured bucket. +* Specifying s3 signer type has been removed, including `cloud.aws.signer` and `cloud.aws.s3.signer`. + ==== Azure Repository plugin * The container an azure repository is configured with will no longer be created automatically. @@ -33,3 +35,7 @@ name space have been removed. This includes `repositories.azure.account`, `repos You must set those settings per repository instead. Respectively `account`, `container`, `base_path`, `location_mode`, `chunk_size` and `compress`. See {plugins}/repository-azure-usage.html#repository-azure-repository-settings[Azure Repository settings]. + +==== EC2 Discovery plugin + +* Specifying ec2 signer type has been removed, including `cloud.aws.signer` and `cloud.aws.ec2.signer`. diff --git a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2Service.java b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2Service.java index 0b1fdca257d..df5debda02b 100644 --- a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2Service.java +++ b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2Service.java @@ -76,10 +76,6 @@ interface AwsEc2Service { */ Setting PROXY_PASSWORD_SETTING = new Setting<>("cloud.aws.proxy.password", "", SecureString::new, Property.NodeScope, Property.Filtered, Property.Shared, Property.Deprecated); - /** - * cloud.aws.signer: If you are using an old AWS API version, you can define a Signer. Shared with repository-s3 plugin - */ - Setting SIGNER_SETTING = Setting.simpleString("cloud.aws.signer", Property.NodeScope, Property.Shared, Property.Deprecated); /** * cloud.aws.region: Region. Shared with repository-s3 plugin */ @@ -140,13 +136,6 @@ interface AwsEc2Service { */ Setting PROXY_PASSWORD_SETTING = new Setting<>("cloud.aws.ec2.proxy.password", AwsEc2Service.PROXY_PASSWORD_SETTING, SecureString::new, Property.NodeScope, Property.Filtered, Property.Deprecated); - /** - * cloud.aws.ec2.signer: If you are using an old AWS API version, you can define a Signer. Specific for EC2 API calls. - * Defaults to cloud.aws.signer. - * @see AwsEc2Service#SIGNER_SETTING - */ - Setting SIGNER_SETTING = new Setting<>("cloud.aws.ec2.signer", AwsEc2Service.SIGNER_SETTING, Function.identity(), - Property.NodeScope, Property.Deprecated); /** * cloud.aws.ec2.region: Region specific for EC2 API calls. Defaults to cloud.aws.region. * @see AwsEc2Service#REGION_SETTING diff --git a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java index 8131b5be9ba..4aa9ec7adb4 100644 --- a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java +++ b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java @@ -104,13 +104,6 @@ class AwsEc2ServiceImpl extends AbstractComponent implements AwsEc2Service, Clos } } - // #155: we might have 3rd party users using older EC2 API version - String awsSigner = CLOUD_EC2.SIGNER_SETTING.get(settings); - if (Strings.hasText(awsSigner)) { - logger.debug("using AWS API signer [{}]", awsSigner); - AwsSigner.configureSigner(awsSigner, clientConfiguration); - } - // Increase the number of retries in case of 5xx API responses final Random rand = Randomness.get(); RetryPolicy retryPolicy = new RetryPolicy( diff --git a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/Ec2DiscoveryPlugin.java b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/Ec2DiscoveryPlugin.java index 5c091b825a4..29babbbeef3 100644 --- a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/Ec2DiscoveryPlugin.java +++ b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/Ec2DiscoveryPlugin.java @@ -127,7 +127,6 @@ public class Ec2DiscoveryPlugin extends Plugin implements DiscoveryPlugin, Close AwsEc2Service.PROXY_PORT_SETTING, AwsEc2Service.PROXY_USERNAME_SETTING, AwsEc2Service.PROXY_PASSWORD_SETTING, - AwsEc2Service.SIGNER_SETTING, AwsEc2Service.REGION_SETTING, AwsEc2Service.READ_TIMEOUT, // Register EC2 specific settings: cloud.aws.ec2 @@ -138,7 +137,6 @@ public class Ec2DiscoveryPlugin extends Plugin implements DiscoveryPlugin, Close AwsEc2Service.CLOUD_EC2.PROXY_PORT_SETTING, AwsEc2Service.CLOUD_EC2.PROXY_USERNAME_SETTING, AwsEc2Service.CLOUD_EC2.PROXY_PASSWORD_SETTING, - AwsEc2Service.CLOUD_EC2.SIGNER_SETTING, AwsEc2Service.CLOUD_EC2.REGION_SETTING, AwsEc2Service.CLOUD_EC2.ENDPOINT_SETTING, AwsEc2Service.CLOUD_EC2.READ_TIMEOUT, diff --git a/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImplTests.java b/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImplTests.java index 0e82d50c17c..13693414bdf 100644 --- a/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImplTests.java +++ b/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImplTests.java @@ -98,7 +98,7 @@ public class AwsEc2ServiceImplTests extends ESTestCase { } public void testAWSDefaultConfiguration() { - launchAWSConfigurationTest(Settings.EMPTY, Protocol.HTTPS, null, -1, null, null, null, + launchAWSConfigurationTest(Settings.EMPTY, Protocol.HTTPS, null, -1, null, null, ClientConfiguration.DEFAULT_SOCKET_TIMEOUT); } @@ -113,8 +113,7 @@ public class AwsEc2ServiceImplTests extends ESTestCase { .put("discovery.ec2.read_timeout", "10s") .setSecureSettings(secureSettings) .build(); - launchAWSConfigurationTest(settings, Protocol.HTTP, "aws_proxy_host", 8080, "aws_proxy_username", "aws_proxy_password", - null, 10000); + launchAWSConfigurationTest(settings, Protocol.HTTP, "aws_proxy_host", 8080, "aws_proxy_username", "aws_proxy_password", 10000); } public void testAWSConfigurationWithAwsSettingsBackcompat() { @@ -124,18 +123,16 @@ public class AwsEc2ServiceImplTests extends ESTestCase { .put(AwsEc2Service.PROXY_PORT_SETTING.getKey(), 8080) .put(AwsEc2Service.PROXY_USERNAME_SETTING.getKey(), "aws_proxy_username") .put(AwsEc2Service.PROXY_PASSWORD_SETTING.getKey(), "aws_proxy_password") - .put(AwsEc2Service.SIGNER_SETTING.getKey(), "AWS3SignerType") .put(AwsEc2Service.READ_TIMEOUT.getKey(), "10s") .build(); launchAWSConfigurationTest(settings, Protocol.HTTP, "aws_proxy_host", 8080, "aws_proxy_username", "aws_proxy_password", - "AWS3SignerType", 10000); + 10000); assertSettingDeprecationsAndWarnings(new Setting[] { AwsEc2Service.PROTOCOL_SETTING, AwsEc2Service.PROXY_HOST_SETTING, AwsEc2Service.PROXY_PORT_SETTING, AwsEc2Service.PROXY_USERNAME_SETTING, AwsEc2Service.PROXY_PASSWORD_SETTING, - AwsEc2Service.SIGNER_SETTING, AwsEc2Service.READ_TIMEOUT }); } @@ -147,32 +144,27 @@ public class AwsEc2ServiceImplTests extends ESTestCase { .put(AwsEc2Service.PROXY_PORT_SETTING.getKey(), 8080) .put(AwsEc2Service.PROXY_USERNAME_SETTING.getKey(), "aws_proxy_username") .put(AwsEc2Service.PROXY_PASSWORD_SETTING.getKey(), "aws_proxy_password") - .put(AwsEc2Service.SIGNER_SETTING.getKey(), "AWS3SignerType") .put(AwsEc2Service.READ_TIMEOUT.getKey(), "20s") .put(AwsEc2Service.CLOUD_EC2.PROTOCOL_SETTING.getKey(), "https") .put(AwsEc2Service.CLOUD_EC2.PROXY_HOST_SETTING.getKey(), "ec2_proxy_host") .put(AwsEc2Service.CLOUD_EC2.PROXY_PORT_SETTING.getKey(), 8081) .put(AwsEc2Service.CLOUD_EC2.PROXY_USERNAME_SETTING.getKey(), "ec2_proxy_username") .put(AwsEc2Service.CLOUD_EC2.PROXY_PASSWORD_SETTING.getKey(), "ec2_proxy_password") - .put(AwsEc2Service.CLOUD_EC2.SIGNER_SETTING.getKey(), "NoOpSignerType") .put(AwsEc2Service.CLOUD_EC2.READ_TIMEOUT.getKey(), "10s") .build(); - launchAWSConfigurationTest(settings, Protocol.HTTPS, "ec2_proxy_host", 8081, "ec2_proxy_username", "ec2_proxy_password", - "NoOpSignerType", 10000); + launchAWSConfigurationTest(settings, Protocol.HTTPS, "ec2_proxy_host", 8081, "ec2_proxy_username", "ec2_proxy_password", 10000); assertSettingDeprecationsAndWarnings(new Setting[] { AwsEc2Service.PROTOCOL_SETTING, AwsEc2Service.PROXY_HOST_SETTING, AwsEc2Service.PROXY_PORT_SETTING, AwsEc2Service.PROXY_USERNAME_SETTING, AwsEc2Service.PROXY_PASSWORD_SETTING, - AwsEc2Service.SIGNER_SETTING, AwsEc2Service.READ_TIMEOUT, AwsEc2Service.CLOUD_EC2.PROTOCOL_SETTING, AwsEc2Service.CLOUD_EC2.PROXY_HOST_SETTING, AwsEc2Service.CLOUD_EC2.PROXY_PORT_SETTING, AwsEc2Service.CLOUD_EC2.PROXY_USERNAME_SETTING, AwsEc2Service.CLOUD_EC2.PROXY_PASSWORD_SETTING, - AwsEc2Service.CLOUD_EC2.SIGNER_SETTING, AwsEc2Service.CLOUD_EC2.READ_TIMEOUT }); } @@ -183,7 +175,6 @@ public class AwsEc2ServiceImplTests extends ESTestCase { int expectedProxyPort, String expectedProxyUsername, String expectedProxyPassword, - String expectedSigner, int expectedReadTimeout) { ClientConfiguration configuration = AwsEc2ServiceImpl.buildConfiguration(logger, settings); @@ -193,7 +184,6 @@ public class AwsEc2ServiceImplTests extends ESTestCase { assertThat(configuration.getProxyPort(), is(expectedProxyPort)); assertThat(configuration.getProxyUsername(), is(expectedProxyUsername)); assertThat(configuration.getProxyPassword(), is(expectedProxyPassword)); - assertThat(configuration.getSignerOverride(), is(expectedSigner)); assertThat(configuration.getSocketTimeout(), is(expectedReadTimeout)); } diff --git a/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/Ec2DiscoverySettingsTests.java b/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/Ec2DiscoverySettingsTests.java index d1ac8cfa0cf..fa287bef712 100644 --- a/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/Ec2DiscoverySettingsTests.java +++ b/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/Ec2DiscoverySettingsTests.java @@ -37,7 +37,6 @@ public class Ec2DiscoverySettingsTests extends ESTestCase { .put(AwsEc2Service.PROXY_PORT_SETTING.getKey(), 10000) .put(AwsEc2Service.PROXY_USERNAME_SETTING.getKey(), "global-proxy-username") .put(AwsEc2Service.PROXY_PASSWORD_SETTING.getKey(), "global-proxy-password") - .put(AwsEc2Service.SIGNER_SETTING.getKey(), "global-signer") .put(AwsEc2Service.REGION_SETTING.getKey(), "global-region") .build(); @@ -49,7 +48,6 @@ public class Ec2DiscoverySettingsTests extends ESTestCase { .put(AwsEc2Service.CLOUD_EC2.PROXY_PORT_SETTING.getKey(), 20000) .put(AwsEc2Service.CLOUD_EC2.PROXY_USERNAME_SETTING.getKey(), "ec2-proxy-username") .put(AwsEc2Service.CLOUD_EC2.PROXY_PASSWORD_SETTING.getKey(), "ec2-proxy-password") - .put(AwsEc2Service.CLOUD_EC2.SIGNER_SETTING.getKey(), "ec2-signer") .put(AwsEc2Service.CLOUD_EC2.REGION_SETTING.getKey(), "ec2-region") .put(AwsEc2Service.CLOUD_EC2.ENDPOINT_SETTING.getKey(), "ec2-endpoint") .build(); @@ -66,7 +64,6 @@ public class Ec2DiscoverySettingsTests extends ESTestCase { assertThat(AwsEc2Service.CLOUD_EC2.PROXY_PORT_SETTING.get(nodeSettings), is(10000)); assertThat(AwsEc2Service.CLOUD_EC2.PROXY_USERNAME_SETTING.get(nodeSettings), is("global-proxy-username")); assertThat(AwsEc2Service.CLOUD_EC2.PROXY_PASSWORD_SETTING.get(nodeSettings), is("global-proxy-password")); - assertThat(AwsEc2Service.CLOUD_EC2.SIGNER_SETTING.get(nodeSettings), is("global-signer")); assertThat(AwsEc2Service.CLOUD_EC2.REGION_SETTING.get(nodeSettings), is("global-region")); assertThat(AwsEc2Service.CLOUD_EC2.ENDPOINT_SETTING.get(nodeSettings), isEmptyString()); assertSettingDeprecationsAndWarnings(new Setting[] { @@ -77,7 +74,6 @@ public class Ec2DiscoverySettingsTests extends ESTestCase { AwsEc2Service.PROXY_PORT_SETTING, AwsEc2Service.PROXY_USERNAME_SETTING, AwsEc2Service.PROXY_PASSWORD_SETTING, - AwsEc2Service.SIGNER_SETTING, AwsEc2Service.REGION_SETTING }); } @@ -94,7 +90,6 @@ public class Ec2DiscoverySettingsTests extends ESTestCase { assertThat(AwsEc2Service.CLOUD_EC2.PROXY_PORT_SETTING.get(nodeSettings), is(20000)); assertThat(AwsEc2Service.CLOUD_EC2.PROXY_USERNAME_SETTING.get(nodeSettings), is("ec2-proxy-username")); assertThat(AwsEc2Service.CLOUD_EC2.PROXY_PASSWORD_SETTING.get(nodeSettings), is("ec2-proxy-password")); - assertThat(AwsEc2Service.CLOUD_EC2.SIGNER_SETTING.get(nodeSettings), is("ec2-signer")); assertThat(AwsEc2Service.CLOUD_EC2.REGION_SETTING.get(nodeSettings), is("ec2-region")); assertThat(AwsEc2Service.CLOUD_EC2.ENDPOINT_SETTING.get(nodeSettings), is("ec2-endpoint")); assertSettingDeprecationsAndWarnings(new Setting[] { @@ -105,7 +100,6 @@ public class Ec2DiscoverySettingsTests extends ESTestCase { AwsEc2Service.PROXY_PORT_SETTING, AwsEc2Service.PROXY_USERNAME_SETTING, AwsEc2Service.PROXY_PASSWORD_SETTING, - AwsEc2Service.SIGNER_SETTING, AwsEc2Service.REGION_SETTING, AwsEc2Service.CLOUD_EC2.KEY_SETTING, AwsEc2Service.CLOUD_EC2.SECRET_SETTING, @@ -114,7 +108,6 @@ public class Ec2DiscoverySettingsTests extends ESTestCase { AwsEc2Service.CLOUD_EC2.PROXY_PORT_SETTING, AwsEc2Service.CLOUD_EC2.PROXY_USERNAME_SETTING, AwsEc2Service.CLOUD_EC2.PROXY_PASSWORD_SETTING, - AwsEc2Service.CLOUD_EC2.SIGNER_SETTING, AwsEc2Service.CLOUD_EC2.REGION_SETTING, AwsEc2Service.CLOUD_EC2.ENDPOINT_SETTING }); diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/AwsS3Service.java b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/AwsS3Service.java index 54404cc4b2d..e91faa2ebf2 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/AwsS3Service.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/AwsS3Service.java @@ -73,11 +73,6 @@ interface AwsS3Service extends LifecycleComponent { */ Setting PROXY_PASSWORD_SETTING = new Setting<>("cloud.aws.proxy.password", "", SecureString::new, Property.NodeScope, Property.Filtered, Property.Deprecated, Property.Shared); - /** - * cloud.aws.signer: If you are using an old AWS API version, you can define a Signer. Shared with discovery-ec2 plugin - */ - Setting SIGNER_SETTING = Setting.simpleString("cloud.aws.signer", - Property.NodeScope, Property.Deprecated, Property.Shared); /** * cloud.aws.read_timeout: Socket read timeout. Shared with discovery-ec2 plugin */ @@ -140,14 +135,6 @@ interface AwsS3Service extends LifecycleComponent { Setting PROXY_PASSWORD_SETTING = new Setting<>("cloud.aws.s3.proxy.password", AwsS3Service.PROXY_PASSWORD_SETTING, SecureString::new, Property.NodeScope, Property.Filtered, Property.Deprecated); - /** - * cloud.aws.s3.signer: If you are using an old AWS API version, you can define a Signer. Specific for S3 API calls. - * Defaults to cloud.aws.signer. - * @see AwsS3Service#SIGNER_SETTING - */ - Setting SIGNER_SETTING = - new Setting<>("cloud.aws.s3.signer", AwsS3Service.SIGNER_SETTING, Function.identity(), - Property.NodeScope, Property.Deprecated); /** * cloud.aws.s3.endpoint: Endpoint. */ diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/InternalAwsS3Service.java b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/InternalAwsS3Service.java index e1ba30baf81..a9dbb61c44d 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/InternalAwsS3Service.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/InternalAwsS3Service.java @@ -147,13 +147,6 @@ class InternalAwsS3Service extends AbstractLifecycleComponent implements AwsS3Se } clientConfiguration.setUseThrottleRetries(useThrottleRetries); - // #155: we might have 3rd party users using older S3 API version - String awsSigner = CLOUD_S3.SIGNER_SETTING.get(settings); - if (Strings.hasText(awsSigner)) { - logger.debug("using AWS API signer [{}]", awsSigner); - AwsSigner.configureSigner(awsSigner, clientConfiguration, endpoint); - } - TimeValue readTimeout = getConfigValue(null, settings, clientName, S3Repository.READ_TIMEOUT_SETTING, null, CLOUD_S3.READ_TIMEOUT); clientConfiguration.setSocketTimeout((int)readTimeout.millis()); diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RepositoryPlugin.java b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RepositoryPlugin.java index 91b12fc2794..d27c3481357 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RepositoryPlugin.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RepositoryPlugin.java @@ -98,7 +98,6 @@ public class S3RepositoryPlugin extends Plugin implements RepositoryPlugin { AwsS3Service.PROXY_PORT_SETTING, AwsS3Service.PROXY_USERNAME_SETTING, AwsS3Service.PROXY_PASSWORD_SETTING, - AwsS3Service.SIGNER_SETTING, AwsS3Service.READ_TIMEOUT, // Register S3 specific settings: cloud.aws.s3 @@ -109,7 +108,6 @@ public class S3RepositoryPlugin extends Plugin implements RepositoryPlugin { AwsS3Service.CLOUD_S3.PROXY_PORT_SETTING, AwsS3Service.CLOUD_S3.PROXY_USERNAME_SETTING, AwsS3Service.CLOUD_S3.PROXY_PASSWORD_SETTING, - AwsS3Service.CLOUD_S3.SIGNER_SETTING, AwsS3Service.CLOUD_S3.ENDPOINT_SETTING, AwsS3Service.CLOUD_S3.READ_TIMEOUT, diff --git a/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/AwsS3ServiceImplTests.java b/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/AwsS3ServiceImplTests.java index 31ee6e2de8b..7d30ffcc1f5 100644 --- a/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/AwsS3ServiceImplTests.java +++ b/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/AwsS3ServiceImplTests.java @@ -184,7 +184,7 @@ public class AwsS3ServiceImplTests extends ESTestCase { } public void testAWSDefaultConfiguration() { - launchAWSConfigurationTest(Settings.EMPTY, Settings.EMPTY, Protocol.HTTPS, null, -1, null, null, null, 3, false, + launchAWSConfigurationTest(Settings.EMPTY, Settings.EMPTY, Protocol.HTTPS, null, -1, null, null, 3, false, ClientConfiguration.DEFAULT_SOCKET_TIMEOUT); } @@ -200,7 +200,7 @@ public class AwsS3ServiceImplTests extends ESTestCase { .put("s3.client.default.read_timeout", "10s") .build(); launchAWSConfigurationTest(settings, Settings.EMPTY, Protocol.HTTP, "aws_proxy_host", 8080, "aws_proxy_username", - "aws_proxy_password", null, 3, false, 10000); + "aws_proxy_password", 3, false, 10000); } public void testAWSConfigurationWithAwsSettingsBackcompat() { @@ -210,18 +210,16 @@ public class AwsS3ServiceImplTests extends ESTestCase { .put(AwsS3Service.PROXY_PORT_SETTING.getKey(), 8080) .put(AwsS3Service.PROXY_USERNAME_SETTING.getKey(), "aws_proxy_username") .put(AwsS3Service.PROXY_PASSWORD_SETTING.getKey(), "aws_proxy_password") - .put(AwsS3Service.SIGNER_SETTING.getKey(), "AWS3SignerType") .put(AwsS3Service.READ_TIMEOUT.getKey(), "10s") .build(); launchAWSConfigurationTest(settings, Settings.EMPTY, Protocol.HTTP, "aws_proxy_host", 8080, "aws_proxy_username", - "aws_proxy_password", "AWS3SignerType", 3, false, 10000); + "aws_proxy_password", 3, false, 10000); assertSettingDeprecationsAndWarnings(new Setting[]{ AwsS3Service.PROXY_USERNAME_SETTING, AwsS3Service.PROXY_PASSWORD_SETTING, AwsS3Service.PROTOCOL_SETTING, AwsS3Service.PROXY_HOST_SETTING, AwsS3Service.PROXY_PORT_SETTING, - AwsS3Service.SIGNER_SETTING, AwsS3Service.READ_TIMEOUT}); } @@ -232,32 +230,28 @@ public class AwsS3ServiceImplTests extends ESTestCase { .put(AwsS3Service.PROXY_PORT_SETTING.getKey(), 8080) .put(AwsS3Service.PROXY_USERNAME_SETTING.getKey(), "aws_proxy_username") .put(AwsS3Service.PROXY_PASSWORD_SETTING.getKey(), "aws_proxy_password") - .put(AwsS3Service.SIGNER_SETTING.getKey(), "AWS3SignerType") .put(AwsS3Service.READ_TIMEOUT.getKey(), "5s") .put(AwsS3Service.CLOUD_S3.PROTOCOL_SETTING.getKey(), "https") .put(AwsS3Service.CLOUD_S3.PROXY_HOST_SETTING.getKey(), "s3_proxy_host") .put(AwsS3Service.CLOUD_S3.PROXY_PORT_SETTING.getKey(), 8081) .put(AwsS3Service.CLOUD_S3.PROXY_USERNAME_SETTING.getKey(), "s3_proxy_username") .put(AwsS3Service.CLOUD_S3.PROXY_PASSWORD_SETTING.getKey(), "s3_proxy_password") - .put(AwsS3Service.CLOUD_S3.SIGNER_SETTING.getKey(), "NoOpSignerType") .put(AwsS3Service.CLOUD_S3.READ_TIMEOUT.getKey(), "10s") .build(); launchAWSConfigurationTest(settings, Settings.EMPTY, Protocol.HTTPS, "s3_proxy_host", 8081, "s3_proxy_username", - "s3_proxy_password", "NoOpSignerType", 3, false, 10000); + "s3_proxy_password", 3, false, 10000); assertSettingDeprecationsAndWarnings(new Setting[] { AwsS3Service.PROXY_USERNAME_SETTING, AwsS3Service.PROXY_PASSWORD_SETTING, AwsS3Service.PROTOCOL_SETTING, AwsS3Service.PROXY_HOST_SETTING, AwsS3Service.PROXY_PORT_SETTING, - AwsS3Service.SIGNER_SETTING, AwsS3Service.READ_TIMEOUT, AwsS3Service.CLOUD_S3.PROXY_USERNAME_SETTING, AwsS3Service.CLOUD_S3.PROXY_PASSWORD_SETTING, AwsS3Service.CLOUD_S3.PROTOCOL_SETTING, AwsS3Service.CLOUD_S3.PROXY_HOST_SETTING, AwsS3Service.CLOUD_S3.PROXY_PORT_SETTING, - AwsS3Service.CLOUD_S3.SIGNER_SETTING, AwsS3Service.CLOUD_S3.READ_TIMEOUT}); } @@ -266,7 +260,7 @@ public class AwsS3ServiceImplTests extends ESTestCase { .put(S3Repository.Repositories.MAX_RETRIES_SETTING.getKey(), 10) .build(); launchAWSConfigurationTest(settings, Settings.EMPTY, Protocol.HTTPS, null, -1, null, - null, null, 10, false, 50000); + null, 10, false, 50000); } public void testRepositoryMaxRetries() { @@ -275,7 +269,7 @@ public class AwsS3ServiceImplTests extends ESTestCase { .put(S3Repository.Repositories.MAX_RETRIES_SETTING.getKey(), 10) .build(); launchAWSConfigurationTest(settings, repositorySettings, Protocol.HTTPS, null, -1, null, - null, null, 20, false, 50000); + null, 20, false, 50000); } protected void launchAWSConfigurationTest(Settings settings, @@ -285,7 +279,6 @@ public class AwsS3ServiceImplTests extends ESTestCase { int expectedProxyPort, String expectedProxyUsername, String expectedProxyPassword, - String expectedSigner, Integer expectedMaxRetries, boolean expectedUseThrottleRetries, int expectedReadTimeout) { @@ -303,7 +296,6 @@ public class AwsS3ServiceImplTests extends ESTestCase { assertThat(configuration.getProxyPort(), is(expectedProxyPort)); assertThat(configuration.getProxyUsername(), is(expectedProxyUsername)); assertThat(configuration.getProxyPassword(), is(expectedProxyPassword)); - assertThat(configuration.getSignerOverride(), is(expectedSigner)); assertThat(configuration.getMaxErrorRetry(), is(expectedMaxRetries)); assertThat(configuration.useThrottledRetries(), is(expectedUseThrottleRetries)); assertThat(configuration.getSocketTimeout(), is(expectedReadTimeout)); From 83ba677e7f565348e69fd91f7b625f2990a7656c Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 7 Apr 2017 22:06:40 -0700 Subject: [PATCH 10/26] Discovery EC2: Remove region setting (#23991) We have both endpoint and region settings. Region was removed from s3 to simplify configuration. This is the ec2 equivalent. closes #22758 --- .../migration/migrate_6_0/plugins.asciidoc | 3 + .../discovery/ec2/AwsEc2Service.java | 11 ---- .../discovery/ec2/AwsEc2ServiceImpl.java | 66 ------------------- .../discovery/ec2/Ec2DiscoveryPlugin.java | 2 - .../discovery/ec2/AwsEc2ServiceImplTests.java | 37 ----------- .../ec2/Ec2DiscoverySettingsTests.java | 7 -- 6 files changed, 3 insertions(+), 123 deletions(-) diff --git a/docs/reference/migration/migrate_6_0/plugins.asciidoc b/docs/reference/migration/migrate_6_0/plugins.asciidoc index e5861887cda..29b63b3aa4f 100644 --- a/docs/reference/migration/migrate_6_0/plugins.asciidoc +++ b/docs/reference/migration/migrate_6_0/plugins.asciidoc @@ -39,3 +39,6 @@ See {plugins}/repository-azure-usage.html#repository-azure-repository-settings[A ==== EC2 Discovery plugin * Specifying ec2 signer type has been removed, including `cloud.aws.signer` and `cloud.aws.ec2.signer`. + +* The region setting has been removed. This includes the settings `cloud.aws.region` +and `cloud.aws.ec2.region`. Instead, specify the full endpoint. diff --git a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2Service.java b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2Service.java index df5debda02b..21baa8fee1e 100644 --- a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2Service.java +++ b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2Service.java @@ -76,11 +76,6 @@ interface AwsEc2Service { */ Setting PROXY_PASSWORD_SETTING = new Setting<>("cloud.aws.proxy.password", "", SecureString::new, Property.NodeScope, Property.Filtered, Property.Shared, Property.Deprecated); - /** - * cloud.aws.region: Region. Shared with repository-s3 plugin - */ - Setting REGION_SETTING = - new Setting<>("cloud.aws.region", "", s -> s.toLowerCase(Locale.ROOT), Property.NodeScope, Property.Shared, Property.Deprecated); /** * cloud.aws.read_timeout: Socket read timeout. Shared with repository-s3 plugin */ @@ -136,12 +131,6 @@ interface AwsEc2Service { */ Setting PROXY_PASSWORD_SETTING = new Setting<>("cloud.aws.ec2.proxy.password", AwsEc2Service.PROXY_PASSWORD_SETTING, SecureString::new, Property.NodeScope, Property.Filtered, Property.Deprecated); - /** - * cloud.aws.ec2.region: Region specific for EC2 API calls. Defaults to cloud.aws.region. - * @see AwsEc2Service#REGION_SETTING - */ - Setting REGION_SETTING = new Setting<>("cloud.aws.ec2.region", AwsEc2Service.REGION_SETTING, - s -> s.toLowerCase(Locale.ROOT), Property.NodeScope, Property.Deprecated); /** * cloud.aws.ec2.endpoint: Endpoint. If not set, endpoint will be guessed based on region setting. */ diff --git a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java index 4aa9ec7adb4..80e4d68949f 100644 --- a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java +++ b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java @@ -131,72 +131,6 @@ class AwsEc2ServiceImpl extends AbstractComponent implements AwsEc2Service, Clos if (DISCOVERY_EC2.ENDPOINT_SETTING.exists(settings) || CLOUD_EC2.ENDPOINT_SETTING.exists(settings)) { endpoint = DISCOVERY_EC2.ENDPOINT_SETTING.get(settings); logger.debug("using explicit ec2 endpoint [{}]", endpoint); - } else if (REGION_SETTING.exists(settings) || CLOUD_EC2.REGION_SETTING.exists(settings)) { - final String region = CLOUD_EC2.REGION_SETTING.get(settings); - switch (region) { - case "us-east-1": - case "us-east": - endpoint = "ec2.us-east-1.amazonaws.com"; - break; - case "us-east-2": - endpoint = "ec2.us-east-2.amazonaws.com"; - break; - case "us-west": - case "us-west-1": - endpoint = "ec2.us-west-1.amazonaws.com"; - break; - case "us-west-2": - endpoint = "ec2.us-west-2.amazonaws.com"; - break; - case "ap-southeast": - case "ap-southeast-1": - endpoint = "ec2.ap-southeast-1.amazonaws.com"; - break; - case "ap-south": - case "ap-south-1": - endpoint = "ec2.ap-south-1.amazonaws.com"; - break; - case "us-gov-west": - case "us-gov-west-1": - endpoint = "ec2.us-gov-west-1.amazonaws.com"; - break; - case "ap-southeast-2": - endpoint = "ec2.ap-southeast-2.amazonaws.com"; - break; - case "ap-northeast": - case "ap-northeast-1": - endpoint = "ec2.ap-northeast-1.amazonaws.com"; - break; - case "ap-northeast-2": - endpoint = "ec2.ap-northeast-2.amazonaws.com"; - break; - case "eu-west": - case "eu-west-1": - endpoint = "ec2.eu-west-1.amazonaws.com"; - break; - case "eu-west-2": - endpoint = "ec2.eu-west-2.amazonaws.com"; - break; - case "eu-central": - case "eu-central-1": - endpoint = "ec2.eu-central-1.amazonaws.com"; - break; - case "sa-east": - case "sa-east-1": - endpoint = "ec2.sa-east-1.amazonaws.com"; - break; - case "cn-north": - case "cn-north-1": - endpoint = "ec2.cn-north-1.amazonaws.com.cn"; - break; - case "ca-central": - case "ca-central-1": - endpoint = "ec2.ca-central-1.amazonaws.com"; - break; - default: - throw new IllegalArgumentException("No automatic endpoint could be derived from region [" + region + "]"); - } - logger.debug("using ec2 region [{}], with endpoint [{}]", region, endpoint); } return endpoint; } diff --git a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/Ec2DiscoveryPlugin.java b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/Ec2DiscoveryPlugin.java index 29babbbeef3..9a871ad5502 100644 --- a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/Ec2DiscoveryPlugin.java +++ b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/Ec2DiscoveryPlugin.java @@ -127,7 +127,6 @@ public class Ec2DiscoveryPlugin extends Plugin implements DiscoveryPlugin, Close AwsEc2Service.PROXY_PORT_SETTING, AwsEc2Service.PROXY_USERNAME_SETTING, AwsEc2Service.PROXY_PASSWORD_SETTING, - AwsEc2Service.REGION_SETTING, AwsEc2Service.READ_TIMEOUT, // Register EC2 specific settings: cloud.aws.ec2 AwsEc2Service.CLOUD_EC2.KEY_SETTING, @@ -137,7 +136,6 @@ public class Ec2DiscoveryPlugin extends Plugin implements DiscoveryPlugin, Close AwsEc2Service.CLOUD_EC2.PROXY_PORT_SETTING, AwsEc2Service.CLOUD_EC2.PROXY_USERNAME_SETTING, AwsEc2Service.CLOUD_EC2.PROXY_PASSWORD_SETTING, - AwsEc2Service.CLOUD_EC2.REGION_SETTING, AwsEc2Service.CLOUD_EC2.ENDPOINT_SETTING, AwsEc2Service.CLOUD_EC2.READ_TIMEOUT, // Register EC2 discovery settings: discovery.ec2 diff --git a/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImplTests.java b/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImplTests.java index 13693414bdf..0f126887d0a 100644 --- a/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImplTests.java +++ b/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImplTests.java @@ -210,41 +210,4 @@ public class AwsEc2ServiceImplTests extends ESTestCase { AwsEc2Service.CLOUD_EC2.ENDPOINT_SETTING }); } - - public void testRegionWithAwsSettings() { - Settings settings = Settings.builder() - .put(AwsEc2Service.REGION_SETTING.getKey(), randomFrom("eu-west", "eu-west-1")) - .build(); - String endpoint = AwsEc2ServiceImpl.findEndpoint(logger, settings); - assertThat(endpoint, is("ec2.eu-west-1.amazonaws.com")); - assertSettingDeprecationsAndWarnings(new Setting[] { - AwsEc2Service.REGION_SETTING - }); - } - - public void testRegionWithAwsAndEc2Settings() { - Settings settings = Settings.builder() - .put(AwsEc2Service.REGION_SETTING.getKey(), randomFrom("eu-west", "eu-west-1")) - .put(AwsEc2Service.CLOUD_EC2.REGION_SETTING.getKey(), randomFrom("us-west", "us-west-1")) - .build(); - String endpoint = AwsEc2ServiceImpl.findEndpoint(logger, settings); - assertThat(endpoint, is("ec2.us-west-1.amazonaws.com")); - assertSettingDeprecationsAndWarnings(new Setting[] { - AwsEc2Service.REGION_SETTING, - AwsEc2Service.CLOUD_EC2.REGION_SETTING - }); - } - - public void testInvalidRegion() { - Settings settings = Settings.builder() - .put(AwsEc2Service.REGION_SETTING.getKey(), "does-not-exist") - .build(); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> { - AwsEc2ServiceImpl.findEndpoint(logger, settings); - }); - assertThat(e.getMessage(), containsString("No automatic endpoint could be derived from region")); - assertSettingDeprecationsAndWarnings(new Setting[] { - AwsEc2Service.REGION_SETTING - }); - } } diff --git a/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/Ec2DiscoverySettingsTests.java b/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/Ec2DiscoverySettingsTests.java index fa287bef712..7918a50da3a 100644 --- a/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/Ec2DiscoverySettingsTests.java +++ b/plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/Ec2DiscoverySettingsTests.java @@ -37,7 +37,6 @@ public class Ec2DiscoverySettingsTests extends ESTestCase { .put(AwsEc2Service.PROXY_PORT_SETTING.getKey(), 10000) .put(AwsEc2Service.PROXY_USERNAME_SETTING.getKey(), "global-proxy-username") .put(AwsEc2Service.PROXY_PASSWORD_SETTING.getKey(), "global-proxy-password") - .put(AwsEc2Service.REGION_SETTING.getKey(), "global-region") .build(); private static final Settings EC2 = Settings.builder() @@ -48,7 +47,6 @@ public class Ec2DiscoverySettingsTests extends ESTestCase { .put(AwsEc2Service.CLOUD_EC2.PROXY_PORT_SETTING.getKey(), 20000) .put(AwsEc2Service.CLOUD_EC2.PROXY_USERNAME_SETTING.getKey(), "ec2-proxy-username") .put(AwsEc2Service.CLOUD_EC2.PROXY_PASSWORD_SETTING.getKey(), "ec2-proxy-password") - .put(AwsEc2Service.CLOUD_EC2.REGION_SETTING.getKey(), "ec2-region") .put(AwsEc2Service.CLOUD_EC2.ENDPOINT_SETTING.getKey(), "ec2-endpoint") .build(); @@ -64,7 +62,6 @@ public class Ec2DiscoverySettingsTests extends ESTestCase { assertThat(AwsEc2Service.CLOUD_EC2.PROXY_PORT_SETTING.get(nodeSettings), is(10000)); assertThat(AwsEc2Service.CLOUD_EC2.PROXY_USERNAME_SETTING.get(nodeSettings), is("global-proxy-username")); assertThat(AwsEc2Service.CLOUD_EC2.PROXY_PASSWORD_SETTING.get(nodeSettings), is("global-proxy-password")); - assertThat(AwsEc2Service.CLOUD_EC2.REGION_SETTING.get(nodeSettings), is("global-region")); assertThat(AwsEc2Service.CLOUD_EC2.ENDPOINT_SETTING.get(nodeSettings), isEmptyString()); assertSettingDeprecationsAndWarnings(new Setting[] { AwsEc2Service.KEY_SETTING, @@ -74,7 +71,6 @@ public class Ec2DiscoverySettingsTests extends ESTestCase { AwsEc2Service.PROXY_PORT_SETTING, AwsEc2Service.PROXY_USERNAME_SETTING, AwsEc2Service.PROXY_PASSWORD_SETTING, - AwsEc2Service.REGION_SETTING }); } @@ -90,7 +86,6 @@ public class Ec2DiscoverySettingsTests extends ESTestCase { assertThat(AwsEc2Service.CLOUD_EC2.PROXY_PORT_SETTING.get(nodeSettings), is(20000)); assertThat(AwsEc2Service.CLOUD_EC2.PROXY_USERNAME_SETTING.get(nodeSettings), is("ec2-proxy-username")); assertThat(AwsEc2Service.CLOUD_EC2.PROXY_PASSWORD_SETTING.get(nodeSettings), is("ec2-proxy-password")); - assertThat(AwsEc2Service.CLOUD_EC2.REGION_SETTING.get(nodeSettings), is("ec2-region")); assertThat(AwsEc2Service.CLOUD_EC2.ENDPOINT_SETTING.get(nodeSettings), is("ec2-endpoint")); assertSettingDeprecationsAndWarnings(new Setting[] { AwsEc2Service.KEY_SETTING, @@ -100,7 +95,6 @@ public class Ec2DiscoverySettingsTests extends ESTestCase { AwsEc2Service.PROXY_PORT_SETTING, AwsEc2Service.PROXY_USERNAME_SETTING, AwsEc2Service.PROXY_PASSWORD_SETTING, - AwsEc2Service.REGION_SETTING, AwsEc2Service.CLOUD_EC2.KEY_SETTING, AwsEc2Service.CLOUD_EC2.SECRET_SETTING, AwsEc2Service.CLOUD_EC2.PROTOCOL_SETTING, @@ -108,7 +102,6 @@ public class Ec2DiscoverySettingsTests extends ESTestCase { AwsEc2Service.CLOUD_EC2.PROXY_PORT_SETTING, AwsEc2Service.CLOUD_EC2.PROXY_USERNAME_SETTING, AwsEc2Service.CLOUD_EC2.PROXY_PASSWORD_SETTING, - AwsEc2Service.CLOUD_EC2.REGION_SETTING, AwsEc2Service.CLOUD_EC2.ENDPOINT_SETTING }); } From 9056e0cb49ed1fe0d56a9e02a91cc47c557b0189 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Sat, 8 Apr 2017 18:22:44 -0400 Subject: [PATCH 11/26] Remove hidden file leniency from plugin service This commit removes some leniency from the plugin service which skips hidden files in the plugins directory. We really want to ensure the integrity of the plugin folder, so hasta la vista leniency. Relates #23982 --- .../elasticsearch/plugins/PluginsService.java | 4 --- .../plugins/PluginsServiceTests.java | 32 +++++++++++++++---- .../migration/migrate_6_0/plugins.asciidoc | 5 +++ 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/plugins/PluginsService.java b/core/src/main/java/org/elasticsearch/plugins/PluginsService.java index fc63678b94f..09f78d36b2b 100644 --- a/core/src/main/java/org/elasticsearch/plugins/PluginsService.java +++ b/core/src/main/java/org/elasticsearch/plugins/PluginsService.java @@ -305,10 +305,6 @@ public class PluginsService extends AbstractComponent { try (DirectoryStream stream = Files.newDirectoryStream(pluginsDirectory)) { for (Path plugin : stream) { - if (FileSystemUtils.isHidden(plugin)) { - logger.trace("--- skip hidden plugin file[{}]", plugin.toAbsolutePath()); - continue; - } logger.trace("--- adding plugin [{}]", plugin.toAbsolutePath()); final PluginInfo info; try { diff --git a/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java b/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java index bb70c58b8c8..5e11746bb25 100644 --- a/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java +++ b/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java @@ -19,17 +19,19 @@ package org.elasticsearch.plugins; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.env.Environment; +import org.elasticsearch.index.IndexModule; +import org.elasticsearch.test.ESTestCase; + +import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; import java.util.Arrays; import java.util.List; -import org.elasticsearch.ElasticsearchException; -import org.elasticsearch.common.inject.AbstractModule; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.env.Environment; -import org.elasticsearch.index.IndexModule; -import org.elasticsearch.test.ESTestCase; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.hasToString; public class PluginsServiceTests extends ESTestCase { public static class AdditionalSettingsPlugin1 extends Plugin { @@ -99,4 +101,22 @@ public class PluginsServiceTests extends ESTestCase { assertEquals(1, scriptPlugins.size()); assertEquals(FilterablePlugin.class, scriptPlugins.get(0).getClass()); } + + public void testHiddenFiles() throws IOException { + final Path home = createTempDir(); + final Settings settings = + Settings.builder() + .put(Environment.PATH_HOME_SETTING.getKey(), home) + .build(); + final Path hidden = home.resolve("plugins").resolve(".hidden"); + Files.createDirectories(hidden); + @SuppressWarnings("unchecked") + final IllegalStateException e = expectThrows( + IllegalStateException.class, + () -> newPluginsService(settings)); + + final String expected = "Could not load plugin descriptor for existing plugin [.hidden]"; + assertThat(e, hasToString(containsString(expected))); + } + } diff --git a/docs/reference/migration/migrate_6_0/plugins.asciidoc b/docs/reference/migration/migrate_6_0/plugins.asciidoc index 29b63b3aa4f..0bc0bb0653d 100644 --- a/docs/reference/migration/migrate_6_0/plugins.asciidoc +++ b/docs/reference/migration/migrate_6_0/plugins.asciidoc @@ -42,3 +42,8 @@ See {plugins}/repository-azure-usage.html#repository-azure-repository-settings[A * The region setting has been removed. This includes the settings `cloud.aws.region` and `cloud.aws.ec2.region`. Instead, specify the full endpoint. + +==== Ignoring hidden folders + +Previous versions of Elasticsearch would skip hidden files and directories when +scanning the plugins folder. This leniency has been removed. From 5c8d5677a4691afcf06b9717cf8e9fb6d252c727 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Sat, 8 Apr 2017 20:42:18 -0400 Subject: [PATCH 12/26] Suppress ExtrasFS in plugins service tests The ExtrasFS filesystem creates extra directories when creating temp directories during tests to ensure that Lucene does not care about extra files. These extra files get in our way in the plugins service tests because some of these tests are counting only on certain directories existing. This commit suppresses the ExtrasFS filesystem for the plugins service tests, and fixes a test that was passing for the wrong reason (because of the existence of an extra directory from ExtrasFS). --- .../java/org/elasticsearch/plugins/PluginsServiceTests.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java b/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java index 5e11746bb25..f4aae5232ce 100644 --- a/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java +++ b/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.plugins; +import org.apache.lucene.util.LuceneTestCase; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.Environment; import org.elasticsearch.index.IndexModule; @@ -33,6 +34,7 @@ import java.util.List; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasToString; +@LuceneTestCase.SuppressFileSystems(value = "ExtrasFS") public class PluginsServiceTests extends ESTestCase { public static class AdditionalSettingsPlugin1 extends Plugin { @Override @@ -87,7 +89,7 @@ public class PluginsServiceTests extends ESTestCase { PluginsService.getPluginBundles(pluginsDir); fail(); } catch (IllegalStateException e) { - assertTrue(e.getMessage(), e.getMessage().contains("Could not load plugin descriptor for existing plugin")); + assertTrue(e.getMessage(), e.getMessage().contains("Could not load plugin descriptor for existing plugin [plugin-missing-descriptor]")); } } From 61c5976aee89f83fbbc181c4388c8360e8b35da0 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Sun, 9 Apr 2017 07:19:16 -0400 Subject: [PATCH 13/26] Upgrade to Log4j 2.8.2 This commit upgrades the Log4j dependencies from version 2.7 to version 2.8.2. This release includes a fix for a case where Log4j could lose exceptions in the presence of a security manager. Relates #23995 --- buildSrc/version.properties | 2 +- core/build.gradle | 2 ++ core/licenses/log4j-1.2-api-2.7.jar.sha1 | 1 - core/licenses/log4j-1.2-api-2.8.2.jar.sha1 | 1 + core/licenses/log4j-api-2.7.jar.sha1 | 1 - core/licenses/log4j-api-2.8.2.jar.sha1 | 1 + core/licenses/log4j-core-2.7.jar.sha1 | 1 - core/licenses/log4j-core-2.8.2.jar.sha1 | 1 + docs/java-api/index.asciidoc | 8 ++++---- 9 files changed, 10 insertions(+), 8 deletions(-) delete mode 100644 core/licenses/log4j-1.2-api-2.7.jar.sha1 create mode 100644 core/licenses/log4j-1.2-api-2.8.2.jar.sha1 delete mode 100644 core/licenses/log4j-api-2.7.jar.sha1 create mode 100644 core/licenses/log4j-api-2.8.2.jar.sha1 delete mode 100644 core/licenses/log4j-core-2.7.jar.sha1 create mode 100644 core/licenses/log4j-core-2.8.2.jar.sha1 diff --git a/buildSrc/version.properties b/buildSrc/version.properties index cea96db283d..a3a1681eb3e 100644 --- a/buildSrc/version.properties +++ b/buildSrc/version.properties @@ -8,7 +8,7 @@ jts = 1.13 jackson = 2.8.6 snakeyaml = 1.15 # When updating log4j, please update also docs/java-api/index.asciidoc -log4j = 2.7 +log4j = 2.8.2 slf4j = 1.6.2 jna = 4.4.0 diff --git a/core/build.gradle b/core/build.gradle index 99da28e2091..8ce1e02ecd4 100644 --- a/core/build.gradle +++ b/core/build.gradle @@ -231,9 +231,11 @@ thirdPartyAudit.excludes = [ 'org.apache.commons.compress.utils.IOUtils', 'org.apache.commons.csv.CSVFormat', 'org.apache.commons.csv.QuoteMode', + 'org.apache.kafka.clients.producer.Callback', 'org.apache.kafka.clients.producer.KafkaProducer', 'org.apache.kafka.clients.producer.Producer', 'org.apache.kafka.clients.producer.ProducerRecord', + 'org.apache.kafka.clients.producer.RecordMetadata', 'org.codehaus.stax2.XMLStreamWriter2', 'org.jctools.queues.MessagePassingQueue$Consumer', 'org.jctools.queues.MpscArrayQueue', diff --git a/core/licenses/log4j-1.2-api-2.7.jar.sha1 b/core/licenses/log4j-1.2-api-2.7.jar.sha1 deleted file mode 100644 index f3644414148..00000000000 --- a/core/licenses/log4j-1.2-api-2.7.jar.sha1 +++ /dev/null @@ -1 +0,0 @@ -39f4e6c2d68d4ef8fd4b0883d165682dedd5be52 \ No newline at end of file diff --git a/core/licenses/log4j-1.2-api-2.8.2.jar.sha1 b/core/licenses/log4j-1.2-api-2.8.2.jar.sha1 new file mode 100644 index 00000000000..39d09bec717 --- /dev/null +++ b/core/licenses/log4j-1.2-api-2.8.2.jar.sha1 @@ -0,0 +1 @@ +f1543534b8413aac91fa54d1fff65dfff76818cd \ No newline at end of file diff --git a/core/licenses/log4j-api-2.7.jar.sha1 b/core/licenses/log4j-api-2.7.jar.sha1 deleted file mode 100644 index 8f676d9dbdd..00000000000 --- a/core/licenses/log4j-api-2.7.jar.sha1 +++ /dev/null @@ -1 +0,0 @@ -8de00e382a817981b737be84cb8def687d392963 \ No newline at end of file diff --git a/core/licenses/log4j-api-2.8.2.jar.sha1 b/core/licenses/log4j-api-2.8.2.jar.sha1 new file mode 100644 index 00000000000..7c7c1da835c --- /dev/null +++ b/core/licenses/log4j-api-2.8.2.jar.sha1 @@ -0,0 +1 @@ +e590eeb783348ce8ddef205b82127f9084d82bf3 \ No newline at end of file diff --git a/core/licenses/log4j-core-2.7.jar.sha1 b/core/licenses/log4j-core-2.7.jar.sha1 deleted file mode 100644 index 07bb057a984..00000000000 --- a/core/licenses/log4j-core-2.7.jar.sha1 +++ /dev/null @@ -1 +0,0 @@ -a3f2b4e64c61a7fc1ed8f1e5ba371933404ed98a \ No newline at end of file diff --git a/core/licenses/log4j-core-2.8.2.jar.sha1 b/core/licenses/log4j-core-2.8.2.jar.sha1 new file mode 100644 index 00000000000..4e6c7b4fcc3 --- /dev/null +++ b/core/licenses/log4j-core-2.8.2.jar.sha1 @@ -0,0 +1 @@ +979fc0cf8460302e4ffbfe38c1b66a99450b0bb7 \ No newline at end of file diff --git a/docs/java-api/index.asciidoc b/docs/java-api/index.asciidoc index 43fc1cacf2d..792404c4149 100644 --- a/docs/java-api/index.asciidoc +++ b/docs/java-api/index.asciidoc @@ -44,12 +44,12 @@ You need to also include Log4j 2 dependencies: org.apache.logging.log4j log4j-api - 2.7 + 2.8.2 org.apache.logging.log4j log4j-core - 2.7 + 2.8.2 -------------------------------------------------- @@ -77,12 +77,12 @@ If you want to use another logger than Log4j 2, you can use http://www.slf4j.org org.apache.logging.log4j log4j-to-slf4j - 2.7 + 2.8.2 org.slf4j slf4j-api - 1.7.21 + 1.7.24 -------------------------------------------------- From f0df5e64d8172c200e9e5e5a8b93a08158e22be2 Mon Sep 17 00:00:00 2001 From: Boaz Leskes Date: Sun, 9 Apr 2017 21:21:00 +0200 Subject: [PATCH 14/26] InternalEngineTests: fix a potential NPE in assertOpsOnPrimary assertOpsOnPrimary may inherit a situation where the document exist but it doesn't the last indexed value. This cloud cause an NPE. --- .../org/elasticsearch/index/engine/InternalEngineTests.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java b/core/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java index 7e5d8d7e73e..88400597061 100644 --- a/core/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java +++ b/core/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java @@ -1560,7 +1560,9 @@ public class InternalEngineTests extends ESTestCase { if (randomBoolean()) { // refresh and take the chance to check everything is ok so far assertVisibleCount(engine, docDeleted ? 0 : 1); - if (docDeleted == false) { + // even if doc is not not deleted, lastFieldValue can still be null if this is the + // first op and it failed. + if (docDeleted == false && lastFieldValue != null) { try (Searcher searcher = engine.acquireSearcher("test")) { final TotalHitCountCollector collector = new TotalHitCountCollector(); searcher.searcher().search(new TermQuery(new Term("value", lastFieldValue)), collector); From b636ca79d579dbef3965b578fa8253aa6189e263 Mon Sep 17 00:00:00 2001 From: Boaz Leskes Date: Sun, 9 Apr 2017 22:04:12 +0200 Subject: [PATCH 15/26] Engine: version logic on replicas should not be hard coded (#23998) The refactoring in #23711 hardcoded version logic for replica to assume monotonic versions. Sadly that's wrong for `FORCE` and `VERSION_GTE`. Instead we should use the methods in VersionType to detect conflicts. Note - once replicas use sequence numbers for out of order delivery, this logic goes away. --- .../index/engine/InternalEngine.java | 4 +- .../index/engine/InternalEngineTests.java | 90 +++++++++++++------ 2 files changed, 65 insertions(+), 29 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/engine/InternalEngine.java b/core/src/main/java/org/elasticsearch/index/engine/InternalEngine.java index aed68d812a6..333dd769eaf 100644 --- a/core/src/main/java/org/elasticsearch/index/engine/InternalEngine.java +++ b/core/src/main/java/org/elasticsearch/index/engine/InternalEngine.java @@ -441,8 +441,8 @@ public class InternalEngine extends Engine { if (versionValue == null) { return OpVsLuceneDocStatus.LUCENE_DOC_NOT_FOUND; } else { - return op.version() > versionValue.getVersion() ? - OpVsLuceneDocStatus.OP_NEWER : OpVsLuceneDocStatus.OP_STALE_OR_EQUAL; + return op.versionType().isVersionConflictForWrites(versionValue.getVersion(), op.version(), versionValue.isDelete()) ? + OpVsLuceneDocStatus.OP_STALE_OR_EQUAL : OpVsLuceneDocStatus.OP_NEWER; } } diff --git a/core/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java b/core/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java index 88400597061..2d3ba055df4 100644 --- a/core/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java +++ b/core/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java @@ -1309,8 +1309,9 @@ public class InternalEngineTests extends ESTestCase { assertThat(indexResult.getFailure(), instanceOf(VersionConflictEngineException.class)); } - protected List generateSingleDocHistory(boolean forReplica, boolean externalVersioning, boolean partialOldPrimary, - long primaryTerm, int minOpCount, int maxOpCount) { + protected List generateSingleDocHistory(boolean forReplica, VersionType versionType, + boolean partialOldPrimary, long primaryTerm, + int minOpCount, int maxOpCount) { final int numOfOps = randomIntBetween(minOpCount, maxOpCount); final List ops = new ArrayList<>(); final Term id = newUid(Uid.createUid("test", "1")); @@ -1322,14 +1323,30 @@ public class InternalEngineTests extends ESTestCase { } final String valuePrefix = forReplica ? "r_" : "p_"; final boolean incrementTermWhenIntroducingSeqNo = randomBoolean(); - final VersionType versionType = externalVersioning ? VersionType.EXTERNAL : VersionType.INTERNAL; for (int i = 0; i < numOfOps; i++) { final Engine.Operation op; + final long version; + switch (versionType) { + case INTERNAL: + version = forReplica ? i : Versions.MATCH_ANY; + break; + case EXTERNAL: + version = i; + break; + case EXTERNAL_GTE: + version = randomBoolean() ? Math.max(i - 1, 0) : i; + break; + case FORCE: + version = randomNonNegativeLong(); + break; + default: + throw new UnsupportedOperationException("unknown version type: " + versionType); + } if (randomBoolean()) { op = new Engine.Index(id, testParsedDocument("1", "test", null, testDocumentWithTextField(valuePrefix + i), B_1, null), forReplica && i >= startWithSeqNo ? i * 2 : SequenceNumbersService.UNASSIGNED_SEQ_NO, forReplica && i >= startWithSeqNo && incrementTermWhenIntroducingSeqNo ? primaryTerm + 1 : primaryTerm, - forReplica || externalVersioning ? i : Versions.MATCH_ANY, + version, forReplica ? versionType.versionTypeForReplicationAndRecovery() : versionType, forReplica ? REPLICA : PRIMARY, System.currentTimeMillis(), -1, false @@ -1338,7 +1355,7 @@ public class InternalEngineTests extends ESTestCase { op = new Engine.Delete("test", "1", id, forReplica && i >= startWithSeqNo ? i * 2 : SequenceNumbersService.UNASSIGNED_SEQ_NO, forReplica && i >= startWithSeqNo && incrementTermWhenIntroducingSeqNo ? primaryTerm + 1 : primaryTerm, - forReplica || externalVersioning ? i : Versions.MATCH_ANY, + version, forReplica ? versionType.versionTypeForReplicationAndRecovery() : versionType, forReplica ? REPLICA : PRIMARY, System.currentTimeMillis()); @@ -1349,10 +1366,20 @@ public class InternalEngineTests extends ESTestCase { } public void testOutOfOrderDocsOnReplica() throws IOException { - final List ops = generateSingleDocHistory(true, true, false, 2, 2, 20); - assertOpsOnReplica(ops, replicaEngine); + final List ops = generateSingleDocHistory(true, + randomFrom(VersionType.INTERNAL, VersionType.EXTERNAL), false, 2, 2, 20); + assertOpsOnReplica(ops, replicaEngine, true); } + public void testNonStandardVersioningOnReplica() throws IOException { + // TODO: this can be folded into testOutOfOrderDocsOnReplica once out of order + // is detected using seq# + final List ops = generateSingleDocHistory(true, + randomFrom(VersionType.EXTERNAL_GTE, VersionType.FORCE), false, 2, 2, 20); + assertOpsOnReplica(ops, replicaEngine, false); + } + + public void testOutOfOrderDocsOnReplicaOldPrimary() throws IOException { IndexSettings oldSettings = IndexSettingsModule.newIndexSettings("testOld", Settings.builder() .put(IndexSettings.INDEX_GC_DELETES_SETTING.getKey(), "1h") // make sure this doesn't kick in on us @@ -1365,12 +1392,12 @@ public class InternalEngineTests extends ESTestCase { try (Store oldReplicaStore = createStore(); InternalEngine replicaEngine = createEngine(oldSettings, oldReplicaStore, createTempDir("translog-old-replica"), newMergePolicy())) { - final List ops = generateSingleDocHistory(true, true, true, 2, 2, 20); - assertOpsOnReplica(ops, replicaEngine); + final List ops = generateSingleDocHistory(true, randomFrom(VersionType.INTERNAL, VersionType.EXTERNAL), true, 2, 2, 20); + assertOpsOnReplica(ops, replicaEngine, true); } } - private void assertOpsOnReplica(List ops, InternalEngine replicaEngine) throws IOException { + private void assertOpsOnReplica(List ops, InternalEngine replicaEngine, boolean shuffleOps) throws IOException { final Engine.Operation lastOp = ops.get(ops.size() - 1); final String lastFieldValue; if (lastOp instanceof Engine.Index) { @@ -1380,13 +1407,15 @@ public class InternalEngineTests extends ESTestCase { // delete lastFieldValue = null; } - int firstOpWithSeqNo = 0; - while (firstOpWithSeqNo < ops.size() && ops.get(firstOpWithSeqNo).seqNo() < 0) { - firstOpWithSeqNo++; + if (shuffleOps) { + int firstOpWithSeqNo = 0; + while (firstOpWithSeqNo < ops.size() && ops.get(firstOpWithSeqNo).seqNo() < 0) { + firstOpWithSeqNo++; + } + // shuffle ops but make sure legacy ops are first + shuffle(ops.subList(0, firstOpWithSeqNo), random()); + shuffle(ops.subList(firstOpWithSeqNo, ops.size()), random()); } - // shuffle ops but make sure legacy ops are first - shuffle(ops.subList(0, firstOpWithSeqNo), random()); - shuffle(ops.subList(firstOpWithSeqNo, ops.size()), random()); boolean firstOp = true; for (Engine.Operation op : ops) { logger.info("performing [{}], v [{}], seq# [{}], term [{}]", @@ -1432,7 +1461,7 @@ public class InternalEngineTests extends ESTestCase { } public void testConcurrentOutOfDocsOnReplica() throws IOException, InterruptedException { - final List ops = generateSingleDocHistory(true, true, false, 2, 100, 300); + final List ops = generateSingleDocHistory(true, randomFrom(VersionType.INTERNAL, VersionType.EXTERNAL), false, 2, 100, 300); final Engine.Operation lastOp = ops.get(ops.size() - 1); final String lastFieldValue; if (lastOp instanceof Engine.Index) { @@ -1492,7 +1521,7 @@ public class InternalEngineTests extends ESTestCase { } public void testInternalVersioningOnPrimary() throws IOException { - final List ops = generateSingleDocHistory(false, false, false, 2, 2, 20); + final List ops = generateSingleDocHistory(false, VersionType.INTERNAL, false, 2, 2, 20); assertOpsOnPrimary(ops, Versions.NOT_FOUND, true, engine); } @@ -1595,8 +1624,11 @@ public class InternalEngineTests extends ESTestCase { return opsPerformed; } - public void testExternalVersioningOnPrimary() throws IOException { - final List ops = generateSingleDocHistory(false, true, false, 2, 2, 20); + public void testNonInternalVersioningOnPrimary() throws IOException { + final Set nonInternalVersioning = new HashSet<>(Arrays.asList(VersionType.values())); + nonInternalVersioning.remove(VersionType.INTERNAL); + final VersionType versionType = randomFrom(nonInternalVersioning); + final List ops = generateSingleDocHistory(false, versionType, false, 2, 2, 20); final Engine.Operation lastOp = ops.get(ops.size() - 1); final String lastFieldValue; if (lastOp instanceof Engine.Index) { @@ -1606,7 +1638,10 @@ public class InternalEngineTests extends ESTestCase { // delete lastFieldValue = null; } - shuffle(ops, random()); + // other version types don't support out of order processing. + if (versionType == VersionType.EXTERNAL) { + shuffle(ops, random()); + } long highestOpVersion = Versions.NOT_FOUND; long seqNo = -1; boolean docDeleted = true; @@ -1616,7 +1651,7 @@ public class InternalEngineTests extends ESTestCase { if (op instanceof Engine.Index) { final Engine.Index index = (Engine.Index) op; Engine.IndexResult result = engine.index(index); - if (op.version() > highestOpVersion) { + if (op.versionType().isVersionConflictForWrites(highestOpVersion, op.version(), docDeleted) == false) { seqNo++; assertThat(result.getSeqNo(), equalTo(seqNo)); assertThat(result.isCreated(), equalTo(docDeleted)); @@ -1634,7 +1669,7 @@ public class InternalEngineTests extends ESTestCase { } else { final Engine.Delete delete = (Engine.Delete) op; Engine.DeleteResult result = engine.delete(delete); - if (op.version() > highestOpVersion) { + if (op.versionType().isVersionConflictForWrites(highestOpVersion, op.version(), docDeleted) == false) { seqNo++; assertThat(result.getSeqNo(), equalTo(seqNo)); assertThat(result.isFound(), equalTo(docDeleted == false)); @@ -1660,6 +1695,7 @@ public class InternalEngineTests extends ESTestCase { assertVisibleCount(engine, docDeleted ? 0 : 1); if (docDeleted == false) { + logger.info("searching for [{}]", lastFieldValue); try (Searcher searcher = engine.acquireSearcher("test")) { final TotalHitCountCollector collector = new TotalHitCountCollector(); searcher.searcher().search(new TermQuery(new Term("value", lastFieldValue)), collector); @@ -1669,13 +1705,13 @@ public class InternalEngineTests extends ESTestCase { } public void testVersioningPromotedReplica() throws IOException { - final List replicaOps = generateSingleDocHistory(true, true, false, 1, 2, 20); - List primaryOps = generateSingleDocHistory(false, false, false, 2, 2, 20); + final List replicaOps = generateSingleDocHistory(true, VersionType.INTERNAL, false, 1, 2, 20); + List primaryOps = generateSingleDocHistory(false, VersionType.INTERNAL, false, 2, 2, 20); Engine.Operation lastReplicaOp = replicaOps.get(replicaOps.size() - 1); final boolean deletedOnReplica = lastReplicaOp instanceof Engine.Delete; final long finalReplicaVersion = lastReplicaOp.version(); final long finalReplicaSeqNo = lastReplicaOp.seqNo(); - assertOpsOnReplica(replicaOps, replicaEngine); + assertOpsOnReplica(replicaOps, replicaEngine, true); final int opsOnPrimary = assertOpsOnPrimary(primaryOps, finalReplicaVersion, deletedOnReplica, replicaEngine); final long currentSeqNo = getSequenceID(replicaEngine, new Engine.Get(false, lastReplicaOp.uid())).v1(); try (Searcher searcher = engine.acquireSearcher("test")) { @@ -1689,7 +1725,7 @@ public class InternalEngineTests extends ESTestCase { } public void testConcurrentExternalVersioningOnPrimary() throws IOException, InterruptedException { - final List ops = generateSingleDocHistory(false, true, false, 2, 100, 300); + final List ops = generateSingleDocHistory(false, VersionType.EXTERNAL, false, 2, 100, 300); final Engine.Operation lastOp = ops.get(ops.size() - 1); final String lastFieldValue; if (lastOp instanceof Engine.Index) { From 1f40f8a2d23759f58ab53c24628c6f340edb5de9 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Mon, 10 Apr 2017 09:37:52 +0200 Subject: [PATCH 16/26] Introduce incremental reduction of TopDocs (#23946) This commit adds support for incremental top N reduction if the number of expected shards in the search request is high enough. The changes here also clean up more code in SearchPhaseController to make the separation between values that are the same on each search result and values that are per response. The reduced search phase result doesn't hold an arbitrary result to obtain values like `from`, `size` or sort values which is now cleanly encapsulated. --- .../action/search/FetchSearchPhase.java | 15 +- .../action/search/SearchPhaseController.java | 297 ++++++++++++------ .../SearchScrollQueryAndFetchAsyncAction.java | 5 +- ...SearchScrollQueryThenFetchAsyncAction.java | 18 +- .../common/util/concurrent/AtomicArray.java | 9 - .../elasticsearch/search/SearchService.java | 4 +- .../search/fetch/FetchPhase.java | 2 +- .../search/query/QueryPhase.java | 67 ++-- .../search/query/QuerySearchResult.java | 60 +++- .../search/SearchPhaseControllerTests.java | 227 ++++++++++--- .../search/SearchServiceTests.java | 19 +- 11 files changed, 493 insertions(+), 230 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/action/search/FetchSearchPhase.java b/core/src/main/java/org/elasticsearch/action/search/FetchSearchPhase.java index 920dd1b0009..a0e313f1d73 100644 --- a/core/src/main/java/org/elasticsearch/action/search/FetchSearchPhase.java +++ b/core/src/main/java/org/elasticsearch/action/search/FetchSearchPhase.java @@ -98,27 +98,26 @@ final class FetchSearchPhase extends SearchPhase { final int numShards = context.getNumShards(); final boolean isScrollSearch = context.getRequest().scroll() != null; List phaseResults = queryResults.asList(); - ScoreDoc[] sortedShardDocs = searchPhaseController.sortDocs(isScrollSearch, phaseResults); String scrollId = isScrollSearch ? TransportSearchHelper.buildScrollId(queryResults) : null; final SearchPhaseController.ReducedQueryPhase reducedQueryPhase = resultConsumer.reduce(); final boolean queryAndFetchOptimization = queryResults.length() == 1; final Runnable finishPhase = () - -> moveToNextPhase(searchPhaseController, sortedShardDocs, scrollId, reducedQueryPhase, queryAndFetchOptimization ? + -> moveToNextPhase(searchPhaseController, scrollId, reducedQueryPhase, queryAndFetchOptimization ? queryResults : fetchResults); if (queryAndFetchOptimization) { assert phaseResults.isEmpty() || phaseResults.get(0).fetchResult() != null; // query AND fetch optimization finishPhase.run(); } else { - final IntArrayList[] docIdsToLoad = searchPhaseController.fillDocIdsToLoad(numShards, sortedShardDocs); - if (sortedShardDocs.length == 0) { // no docs to fetch -- sidestep everything and return + final IntArrayList[] docIdsToLoad = searchPhaseController.fillDocIdsToLoad(numShards, reducedQueryPhase.scoreDocs); + if (reducedQueryPhase.scoreDocs.length == 0) { // no docs to fetch -- sidestep everything and return phaseResults.stream() .map(e -> e.queryResult()) .forEach(this::releaseIrrelevantSearchContext); // we have to release contexts here to free up resources finishPhase.run(); } else { final ScoreDoc[] lastEmittedDocPerShard = isScrollSearch ? - searchPhaseController.getLastEmittedDocPerShard(reducedQueryPhase, sortedShardDocs, numShards) + searchPhaseController.getLastEmittedDocPerShard(reducedQueryPhase, numShards) : null; final CountedCollector counter = new CountedCollector<>(r -> fetchResults.set(r.getShardIndex(), r), docIdsToLoad.length, // we count down every shard in the result no matter if we got any results or not @@ -188,7 +187,7 @@ final class FetchSearchPhase extends SearchPhase { private void releaseIrrelevantSearchContext(QuerySearchResult queryResult) { // we only release search context that we did not fetch from if we are not scrolling // and if it has at lease one hit that didn't make it to the global topDocs - if (context.getRequest().scroll() == null && queryResult.hasHits()) { + if (context.getRequest().scroll() == null && queryResult.hasSearchContext()) { try { Transport.Connection connection = context.getConnection(queryResult.getSearchShardTarget().getNodeId()); context.sendReleaseSearchContext(queryResult.getRequestId(), connection); @@ -198,11 +197,11 @@ final class FetchSearchPhase extends SearchPhase { } } - private void moveToNextPhase(SearchPhaseController searchPhaseController, ScoreDoc[] sortedDocs, + private void moveToNextPhase(SearchPhaseController searchPhaseController, String scrollId, SearchPhaseController.ReducedQueryPhase reducedQueryPhase, AtomicArray fetchResultsArr) { final InternalSearchResponse internalResponse = searchPhaseController.merge(context.getRequest().scroll() != null, - sortedDocs, reducedQueryPhase, fetchResultsArr.asList(), fetchResultsArr::get); + reducedQueryPhase, fetchResultsArr.asList(), fetchResultsArr::get); context.executeNextPhase(this, nextPhaseFactory.apply(context.buildSearchResponse(internalResponse, scrollId))); } diff --git a/core/src/main/java/org/elasticsearch/action/search/SearchPhaseController.java b/core/src/main/java/org/elasticsearch/action/search/SearchPhaseController.java index 38d793b93ed..13b4b2f73d4 100644 --- a/core/src/main/java/org/elasticsearch/action/search/SearchPhaseController.java +++ b/core/src/main/java/org/elasticsearch/action/search/SearchPhaseController.java @@ -36,6 +36,7 @@ import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.BigArrays; import org.elasticsearch.script.ScriptService; +import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.SearchHit; import org.elasticsearch.search.SearchHits; import org.elasticsearch.search.SearchPhaseResult; @@ -56,7 +57,6 @@ import org.elasticsearch.search.suggest.Suggest.Suggestion; import org.elasticsearch.search.suggest.Suggest.Suggestion.Entry; import org.elasticsearch.search.suggest.completion.CompletionSuggestion; -import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -147,42 +147,47 @@ public final class SearchPhaseController extends AbstractComponent { * @param ignoreFrom Whether to ignore the from and sort all hits in each shard result. * Enabled only for scroll search, because that only retrieves hits of length 'size' in the query phase. * @param results the search phase results to obtain the sort docs from + * @param bufferedTopDocs the pre-consumed buffered top docs + * @param topDocsStats the top docs stats to fill + * @param from the offset into the search results top docs + * @param size the number of hits to return from the merged top docs */ - public ScoreDoc[] sortDocs(boolean ignoreFrom, Collection results) throws IOException { + public SortedTopDocs sortDocs(boolean ignoreFrom, Collection results, + final Collection bufferedTopDocs, final TopDocsStats topDocsStats, int from, int size) { if (results.isEmpty()) { - return EMPTY_DOCS; + return SortedTopDocs.EMPTY; } - final Collection topDocs = new ArrayList<>(); + final Collection topDocs = bufferedTopDocs == null ? new ArrayList<>() : bufferedTopDocs; final Map>> groupedCompletionSuggestions = new HashMap<>(); - int from = -1; - int size = -1; - for (SearchPhaseResult sortedResult : results) { + for (SearchPhaseResult sortedResult : results) { // TODO we can move this loop into the reduce call to only loop over this once /* We loop over all results once, group together the completion suggestions if there are any and collect relevant * top docs results. Each top docs gets it's shard index set on all top docs to simplify top docs merging down the road * this allowed to remove a single shared optimization code here since now we don't materialized a dense array of * top docs anymore but instead only pass relevant results / top docs to the merge method*/ QuerySearchResult queryResult = sortedResult.queryResult(); - if (queryResult.hasHits()) { - from = queryResult.from(); - size = queryResult.size(); - TopDocs td = queryResult.topDocs(); - if (td != null && td.scoreDocs.length > 0) { + if (queryResult.hasConsumedTopDocs() == false) { // already consumed? + final TopDocs td = queryResult.consumeTopDocs(); + assert td != null; + topDocsStats.add(td); + if (td.scoreDocs.length > 0) { // make sure we set the shard index before we add it - the consumer didn't do that yet setShardIndex(td, queryResult.getShardIndex()); topDocs.add(td); } + } + if (queryResult.hasSuggestHits()) { Suggest shardSuggest = queryResult.suggest(); - if (shardSuggest != null) { - for (CompletionSuggestion suggestion : shardSuggest.filter(CompletionSuggestion.class)) { - suggestion.setShardIndex(sortedResult.getShardIndex()); - List> suggestions = - groupedCompletionSuggestions.computeIfAbsent(suggestion.getName(), s -> new ArrayList<>()); - suggestions.add(suggestion); - } + for (CompletionSuggestion suggestion : shardSuggest.filter(CompletionSuggestion.class)) { + suggestion.setShardIndex(sortedResult.getShardIndex()); + List> suggestions = + groupedCompletionSuggestions.computeIfAbsent(suggestion.getName(), s -> new ArrayList<>()); + suggestions.add(suggestion); } } } - if (size != -1) { - final ScoreDoc[] mergedScoreDocs = mergeTopDocs(topDocs, size, ignoreFrom ? 0 : from); + final boolean hasHits = (groupedCompletionSuggestions.isEmpty() && topDocs.isEmpty()) == false; + if (hasHits) { + final TopDocs mergedTopDocs = mergeTopDocs(topDocs, size, ignoreFrom ? 0 : from); + final ScoreDoc[] mergedScoreDocs = mergedTopDocs == null ? EMPTY_DOCS : mergedTopDocs.scoreDocs; ScoreDoc[] scoreDocs = mergedScoreDocs; if (groupedCompletionSuggestions.isEmpty() == false) { int numSuggestDocs = 0; @@ -204,23 +209,35 @@ public final class SearchPhaseController extends AbstractComponent { } } } - return scoreDocs; + final boolean isSortedByField; + final SortField[] sortFields; + if (mergedTopDocs != null && mergedTopDocs instanceof TopFieldDocs) { + TopFieldDocs fieldDocs = (TopFieldDocs) mergedTopDocs; + isSortedByField = (fieldDocs instanceof CollapseTopFieldDocs && + fieldDocs.fields.length == 1 && fieldDocs.fields[0].getType() == SortField.Type.SCORE) == false; + sortFields = fieldDocs.fields; + } else { + isSortedByField = false; + sortFields = null; + } + return new SortedTopDocs(scoreDocs, isSortedByField, sortFields); } else { - // no relevant docs - just return an empty array - return EMPTY_DOCS; + // no relevant docs + return SortedTopDocs.EMPTY; } } - private ScoreDoc[] mergeTopDocs(Collection results, int topN, int from) { + TopDocs mergeTopDocs(Collection results, int topN, int from) { if (results.isEmpty()) { - return EMPTY_DOCS; + return null; } + assert results.isEmpty() == false; final boolean setShardIndex = false; final TopDocs topDocs = results.stream().findFirst().get(); final TopDocs mergedTopDocs; final int numShards = results.size(); if (numShards == 1 && from == 0) { // only one shard and no pagination we can just return the topDocs as we got them. - return topDocs.scoreDocs; + return topDocs; } else if (topDocs instanceof CollapseTopFieldDocs) { CollapseTopFieldDocs firstTopDocs = (CollapseTopFieldDocs) topDocs; final Sort sort = new Sort(firstTopDocs.fields); @@ -235,7 +252,7 @@ public final class SearchPhaseController extends AbstractComponent { final TopDocs[] shardTopDocs = results.toArray(new TopDocs[numShards]); mergedTopDocs = TopDocs.merge(from, topN, shardTopDocs, setShardIndex); } - return mergedTopDocs.scoreDocs; + return mergedTopDocs; } private static void setShardIndex(TopDocs topDocs, int shardIndex) { @@ -249,12 +266,12 @@ public final class SearchPhaseController extends AbstractComponent { } } - public ScoreDoc[] getLastEmittedDocPerShard(ReducedQueryPhase reducedQueryPhase, - ScoreDoc[] sortedScoreDocs, int numShards) { - ScoreDoc[] lastEmittedDocPerShard = new ScoreDoc[numShards]; - if (reducedQueryPhase.isEmpty() == false) { + public ScoreDoc[] getLastEmittedDocPerShard(ReducedQueryPhase reducedQueryPhase, int numShards) { + final ScoreDoc[] lastEmittedDocPerShard = new ScoreDoc[numShards]; + if (reducedQueryPhase.isEmptyResult == false) { + final ScoreDoc[] sortedScoreDocs = reducedQueryPhase.scoreDocs; // from is always zero as when we use scroll, we ignore from - long size = Math.min(reducedQueryPhase.fetchHits, reducedQueryPhase.oneResult.size()); + long size = Math.min(reducedQueryPhase.fetchHits, reducedQueryPhase.size); // with collapsing we can have more hits than sorted docs size = Math.min(sortedScoreDocs.length, size); for (int sortedDocsIndex = 0; sortedDocsIndex < size; sortedDocsIndex++) { @@ -288,13 +305,13 @@ public final class SearchPhaseController extends AbstractComponent { * Expects sortedDocs to have top search docs across all shards, optionally followed by top suggest docs for each named * completion suggestion ordered by suggestion name */ - public InternalSearchResponse merge(boolean ignoreFrom, ScoreDoc[] sortedDocs, - ReducedQueryPhase reducedQueryPhase, + public InternalSearchResponse merge(boolean ignoreFrom, ReducedQueryPhase reducedQueryPhase, Collection fetchResults, IntFunction resultsLookup) { - if (reducedQueryPhase.isEmpty()) { + if (reducedQueryPhase.isEmptyResult) { return InternalSearchResponse.empty(); } - SearchHits hits = getHits(reducedQueryPhase, ignoreFrom, sortedDocs, fetchResults, resultsLookup); + ScoreDoc[] sortedDocs = reducedQueryPhase.scoreDocs; + SearchHits hits = getHits(reducedQueryPhase, ignoreFrom, fetchResults, resultsLookup); if (reducedQueryPhase.suggest != null) { if (!fetchResults.isEmpty()) { int currentOffset = hits.getHits().length; @@ -329,21 +346,15 @@ public final class SearchPhaseController extends AbstractComponent { return reducedQueryPhase.buildResponse(hits); } - private SearchHits getHits(ReducedQueryPhase reducedQueryPhase, boolean ignoreFrom, ScoreDoc[] sortedDocs, + private SearchHits getHits(ReducedQueryPhase reducedQueryPhase, boolean ignoreFrom, Collection fetchResults, IntFunction resultsLookup) { - boolean sorted = false; + final boolean sorted = reducedQueryPhase.isSortedByField; + ScoreDoc[] sortedDocs = reducedQueryPhase.scoreDocs; int sortScoreIndex = -1; - if (reducedQueryPhase.oneResult.topDocs() instanceof TopFieldDocs) { - TopFieldDocs fieldDocs = (TopFieldDocs) reducedQueryPhase.oneResult.queryResult().topDocs(); - if (fieldDocs instanceof CollapseTopFieldDocs && - fieldDocs.fields.length == 1 && fieldDocs.fields[0].getType() == SortField.Type.SCORE) { - sorted = false; - } else { - sorted = true; - for (int i = 0; i < fieldDocs.fields.length; i++) { - if (fieldDocs.fields[i].getType() == SortField.Type.SCORE) { - sortScoreIndex = i; - } + if (sorted) { + for (int i = 0; i < reducedQueryPhase.sortField.length; i++) { + if (reducedQueryPhase.sortField[i].getType() == SortField.Type.SCORE) { + sortScoreIndex = i; } } } @@ -351,8 +362,8 @@ public final class SearchPhaseController extends AbstractComponent { for (SearchPhaseResult entry : fetchResults) { entry.fetchResult().initCounter(); } - int from = ignoreFrom ? 0 : reducedQueryPhase.oneResult.queryResult().from(); - int numSearchHits = (int) Math.min(reducedQueryPhase.fetchHits - from, reducedQueryPhase.oneResult.size()); + int from = ignoreFrom ? 0 : reducedQueryPhase.from; + int numSearchHits = (int) Math.min(reducedQueryPhase.fetchHits - from, reducedQueryPhase.size); // with collapsing we can have more fetch hits than sorted docs numSearchHits = Math.min(sortedDocs.length, numSearchHits); // merge hits @@ -376,7 +387,7 @@ public final class SearchPhaseController extends AbstractComponent { searchHit.shard(fetchResult.getSearchShardTarget()); if (sorted) { FieldDoc fieldDoc = (FieldDoc) shardDoc; - searchHit.sortValues(fieldDoc.fields, reducedQueryPhase.oneResult.sortValueFormats()); + searchHit.sortValues(fieldDoc.fields, reducedQueryPhase.sortValueFormats); if (sortScoreIndex != -1) { searchHit.score(((Number) fieldDoc.fields[sortScoreIndex]).floatValue()); } @@ -393,42 +404,42 @@ public final class SearchPhaseController extends AbstractComponent { * Reduces the given query results and consumes all aggregations and profile results. * @param queryResults a list of non-null query shard results */ - public ReducedQueryPhase reducedQueryPhase(List queryResults) { - return reducedQueryPhase(queryResults, null, 0); + public ReducedQueryPhase reducedQueryPhase(Collection queryResults, boolean isScrollRequest) { + return reducedQueryPhase(queryResults, null, new ArrayList<>(), new TopDocsStats(), 0, isScrollRequest); } /** * Reduces the given query results and consumes all aggregations and profile results. * @param queryResults a list of non-null query shard results - * @param bufferdAggs a list of pre-collected / buffered aggregations. if this list is non-null all aggregations have been consumed + * @param bufferedAggs a list of pre-collected / buffered aggregations. if this list is non-null all aggregations have been consumed + * from all non-null query results. + * @param bufferedTopDocs a list of pre-collected / buffered top docs. if this list is non-null all top docs have been consumed * from all non-null query results. * @param numReducePhases the number of non-final reduce phases applied to the query results. * @see QuerySearchResult#consumeAggs() * @see QuerySearchResult#consumeProfileResult() */ private ReducedQueryPhase reducedQueryPhase(Collection queryResults, - List bufferdAggs, int numReducePhases) { + List bufferedAggs, List bufferedTopDocs, + TopDocsStats topDocsStats, int numReducePhases, boolean isScrollRequest) { assert numReducePhases >= 0 : "num reduce phases must be >= 0 but was: " + numReducePhases; numReducePhases++; // increment for this phase - long totalHits = 0; - long fetchHits = 0; - float maxScore = Float.NEGATIVE_INFINITY; boolean timedOut = false; Boolean terminatedEarly = null; if (queryResults.isEmpty()) { // early terminate we have nothing to reduce - return new ReducedQueryPhase(totalHits, fetchHits, maxScore, timedOut, terminatedEarly, null, null, null, null, - numReducePhases); + return new ReducedQueryPhase(topDocsStats.totalHits, topDocsStats.fetchHits, topDocsStats.maxScore, + timedOut, terminatedEarly, null, null, null, EMPTY_DOCS, null, null, numReducePhases, false, 0, 0, true); } final QuerySearchResult firstResult = queryResults.stream().findFirst().get().queryResult(); final boolean hasSuggest = firstResult.suggest() != null; final boolean hasProfileResults = firstResult.hasProfileResults(); final boolean consumeAggs; final List aggregationsList; - if (bufferdAggs != null) { + if (bufferedAggs != null) { consumeAggs = false; // we already have results from intermediate reduces and just need to perform the final reduce assert firstResult.hasAggs() : "firstResult has no aggs but we got non null buffered aggs?"; - aggregationsList = bufferdAggs; + aggregationsList = bufferedAggs; } else if (firstResult.hasAggs()) { // the number of shards was less than the buffer size so we reduce agg results directly aggregationsList = new ArrayList<>(queryResults.size()); @@ -443,8 +454,12 @@ public final class SearchPhaseController extends AbstractComponent { final Map> groupedSuggestions = hasSuggest ? new HashMap<>() : Collections.emptyMap(); final Map profileResults = hasProfileResults ? new HashMap<>(queryResults.size()) : Collections.emptyMap(); + int from = 0; + int size = 0; for (SearchPhaseResult entry : queryResults) { QuerySearchResult result = entry.queryResult(); + from = result.from(); + size = result.size(); if (result.searchTimedOut()) { timedOut = true; } @@ -455,11 +470,6 @@ public final class SearchPhaseController extends AbstractComponent { terminatedEarly = true; } } - totalHits += result.topDocs().totalHits; - fetchHits += result.topDocs().scoreDocs.length; - if (!Float.isNaN(result.topDocs().getMaxScore())) { - maxScore = Math.max(maxScore, result.topDocs().getMaxScore()); - } if (hasSuggest) { assert result.suggest() != null; for (Suggestion> suggestion : result.suggest()) { @@ -480,8 +490,11 @@ public final class SearchPhaseController extends AbstractComponent { final InternalAggregations aggregations = aggregationsList.isEmpty() ? null : reduceAggs(aggregationsList, firstResult.pipelineAggregators(), reduceContext); final SearchProfileShardResults shardResults = profileResults.isEmpty() ? null : new SearchProfileShardResults(profileResults); - return new ReducedQueryPhase(totalHits, fetchHits, maxScore, timedOut, terminatedEarly, firstResult, suggest, aggregations, - shardResults, numReducePhases); + final SortedTopDocs scoreDocs = this.sortDocs(isScrollRequest, queryResults, bufferedTopDocs, topDocsStats, from, size); + return new ReducedQueryPhase(topDocsStats.totalHits, topDocsStats.fetchHits, topDocsStats.maxScore, + timedOut, terminatedEarly, suggest, aggregations, shardResults, scoreDocs.scoreDocs, scoreDocs.sortFields, + firstResult != null ? firstResult.sortValueFormats() : null, + numReducePhases, scoreDocs.isSortedByField, size, from, firstResult == null); } @@ -522,8 +535,6 @@ public final class SearchPhaseController extends AbstractComponent { final boolean timedOut; // non null and true if at least one reduced result was terminated early final Boolean terminatedEarly; - // an non-null arbitrary query result if was at least one reduced result - final QuerySearchResult oneResult; // the reduced suggest results final Suggest suggest; // the reduced internal aggregations @@ -532,10 +543,25 @@ public final class SearchPhaseController extends AbstractComponent { final SearchProfileShardResults shardResults; // the number of reduces phases final int numReducePhases; + // the searches merged top docs + final ScoreDoc[] scoreDocs; + // the top docs sort fields used to sort the score docs, null if the results are not sorted + final SortField[] sortField; + // true iff the result score docs is sorted by a field (not score), this implies that sortField is set. + final boolean isSortedByField; + // the size of the top hits to return + final int size; + // true iff the query phase had no results. Otherwise false + final boolean isEmptyResult; + // the offset into the merged top hits + final int from; + // sort value formats used to sort / format the result + final DocValueFormat[] sortValueFormats; - ReducedQueryPhase(long totalHits, long fetchHits, float maxScore, boolean timedOut, Boolean terminatedEarly, - QuerySearchResult oneResult, Suggest suggest, InternalAggregations aggregations, - SearchProfileShardResults shardResults, int numReducePhases) { + ReducedQueryPhase(long totalHits, long fetchHits, float maxScore, boolean timedOut, Boolean terminatedEarly, Suggest suggest, + InternalAggregations aggregations, SearchProfileShardResults shardResults, ScoreDoc[] scoreDocs, + SortField[] sortFields, DocValueFormat[] sortValueFormats, int numReducePhases, boolean isSortedByField, int size, + int from, boolean isEmptyResult) { if (numReducePhases <= 0) { throw new IllegalArgumentException("at least one reduce phase must have been applied but was: " + numReducePhases); } @@ -548,27 +574,26 @@ public final class SearchPhaseController extends AbstractComponent { } this.timedOut = timedOut; this.terminatedEarly = terminatedEarly; - this.oneResult = oneResult; this.suggest = suggest; this.aggregations = aggregations; this.shardResults = shardResults; this.numReducePhases = numReducePhases; + this.scoreDocs = scoreDocs; + this.sortField = sortFields; + this.isSortedByField = isSortedByField; + this.size = size; + this.from = from; + this.isEmptyResult = isEmptyResult; + this.sortValueFormats = sortValueFormats; } /** * Creates a new search response from the given merged hits. - * @see #merge(boolean, ScoreDoc[], ReducedQueryPhase, Collection, IntFunction) + * @see #merge(boolean, ReducedQueryPhase, Collection, IntFunction) */ public InternalSearchResponse buildResponse(SearchHits hits) { return new InternalSearchResponse(hits, aggregations, suggest, shardResults, timedOut, terminatedEarly, numReducePhases); } - - /** - * Returns true iff the query phase had no results. Otherwise false - */ - public boolean isEmpty() { - return oneResult == null; - } } /** @@ -577,12 +602,16 @@ public final class SearchPhaseController extends AbstractComponent { * This implementation can be configured to batch up a certain amount of results and only reduce them * iff the buffer is exhausted. */ - static final class QueryPhaseResultConsumer - extends InitialSearchPhase.SearchPhaseResults { - private final InternalAggregations[] buffer; + static final class QueryPhaseResultConsumer extends InitialSearchPhase.SearchPhaseResults { + private final InternalAggregations[] aggsBuffer; + private final TopDocs[] topDocsBuffer; + private final boolean hasAggs; + private final boolean hasTopDocs; + private final int bufferSize; private int index; private final SearchPhaseController controller; private int numReducePhases = 0; + private final TopDocsStats topDocsStats = new TopDocsStats(); /** * Creates a new {@link QueryPhaseResultConsumer} @@ -591,7 +620,8 @@ public final class SearchPhaseController extends AbstractComponent { * @param bufferSize the size of the reduce buffer. if the buffer size is smaller than the number of expected results * the buffer is used to incrementally reduce aggregation results before all shards responded. */ - private QueryPhaseResultConsumer(SearchPhaseController controller, int expectedResultSize, int bufferSize) { + private QueryPhaseResultConsumer(SearchPhaseController controller, int expectedResultSize, int bufferSize, + boolean hasTopDocs, boolean hasAggs) { super(expectedResultSize); if (expectedResultSize != 1 && bufferSize < 2) { throw new IllegalArgumentException("buffer size must be >= 2 if there is more than one expected result"); @@ -599,39 +629,68 @@ public final class SearchPhaseController extends AbstractComponent { if (expectedResultSize <= bufferSize) { throw new IllegalArgumentException("buffer size must be less than the expected result size"); } + if (hasAggs == false && hasTopDocs == false) { + throw new IllegalArgumentException("either aggs or top docs must be present"); + } this.controller = controller; // no need to buffer anything if we have less expected results. in this case we don't consume any results ahead of time. - this.buffer = new InternalAggregations[bufferSize]; + this.aggsBuffer = new InternalAggregations[hasAggs ? bufferSize : 0]; + this.topDocsBuffer = new TopDocs[hasTopDocs ? bufferSize : 0]; + this.hasTopDocs = hasTopDocs; + this.hasAggs = hasAggs; + this.bufferSize = bufferSize; + } @Override public void consumeResult(SearchPhaseResult result) { super.consumeResult(result); QuerySearchResult queryResult = result.queryResult(); - assert queryResult.hasAggs() : "this collector should only be used if aggs are requested"; consumeInternal(queryResult); } private synchronized void consumeInternal(QuerySearchResult querySearchResult) { - InternalAggregations aggregations = (InternalAggregations) querySearchResult.consumeAggs(); - if (index == buffer.length) { - InternalAggregations reducedAggs = controller.reduceAggsIncrementally(Arrays.asList(buffer)); - Arrays.fill(buffer, null); + if (index == bufferSize) { + if (hasAggs) { + InternalAggregations reducedAggs = controller.reduceAggsIncrementally(Arrays.asList(aggsBuffer)); + Arrays.fill(aggsBuffer, null); + aggsBuffer[0] = reducedAggs; + } + if (hasTopDocs) { + TopDocs reducedTopDocs = controller.mergeTopDocs(Arrays.asList(topDocsBuffer), + querySearchResult.from() + querySearchResult.size() // we have to merge here in the same way we collect on a shard + , 0); + Arrays.fill(topDocsBuffer, null); + topDocsBuffer[0] = reducedTopDocs; + } numReducePhases++; - buffer[0] = reducedAggs; index = 1; } final int i = index++; - buffer[i] = aggregations; + if (hasAggs) { + aggsBuffer[i] = (InternalAggregations) querySearchResult.consumeAggs(); + } + if (hasTopDocs) { + final TopDocs topDocs = querySearchResult.consumeTopDocs(); // can't be null + topDocsStats.add(topDocs); + SearchPhaseController.setShardIndex(topDocs, querySearchResult.getShardIndex()); + topDocsBuffer[i] = topDocs; + } } - private synchronized List getRemaining() { - return Arrays.asList(buffer).subList(0, index); + private synchronized List getRemainingAggs() { + return hasAggs ? Arrays.asList(aggsBuffer).subList(0, index) : null; } + private synchronized List getRemainingTopDocs() { + return hasTopDocs ? Arrays.asList(topDocsBuffer).subList(0, index) : null; + } + + @Override public ReducedQueryPhase reduce() { - return controller.reducedQueryPhase(results.asList(), getRemaining(), numReducePhases); + return controller.reducedQueryPhase(results.asList(), getRemainingAggs(), getRemainingTopDocs(), topDocsStats, + numReducePhases, false); } /** @@ -649,17 +708,49 @@ public final class SearchPhaseController extends AbstractComponent { */ InitialSearchPhase.SearchPhaseResults newSearchPhaseResults(SearchRequest request, int numShards) { SearchSourceBuilder source = request.source(); - if (source != null && source.aggregations() != null) { + boolean isScrollRequest = request.scroll() != null; + final boolean hasAggs = source != null && source.aggregations() != null; + final boolean hasTopDocs = source == null || source.size() != 0; + + if (isScrollRequest == false && (hasAggs || hasTopDocs)) { + // no incremental reduce if scroll is used - we only hit a single shard or sometimes more... if (request.getBatchedReduceSize() < numShards) { // only use this if there are aggs and if there are more shards than we should reduce at once - return new QueryPhaseResultConsumer(this, numShards, request.getBatchedReduceSize()); + return new QueryPhaseResultConsumer(this, numShards, request.getBatchedReduceSize(), hasTopDocs, hasAggs); } } return new InitialSearchPhase.SearchPhaseResults(numShards) { @Override public ReducedQueryPhase reduce() { - return reducedQueryPhase(results.asList()); + return reducedQueryPhase(results.asList(), isScrollRequest); } }; } + + static final class TopDocsStats { + long totalHits; + long fetchHits; + float maxScore = Float.NEGATIVE_INFINITY; + + void add(TopDocs topDocs) { + totalHits += topDocs.totalHits; + fetchHits += topDocs.scoreDocs.length; + if (!Float.isNaN(topDocs.getMaxScore())) { + maxScore = Math.max(maxScore, topDocs.getMaxScore()); + } + } + } + + static final class SortedTopDocs { + static final SortedTopDocs EMPTY = new SortedTopDocs(EMPTY_DOCS, false, null); + final ScoreDoc[] scoreDocs; + final boolean isSortedByField; + final SortField[] sortFields; + + SortedTopDocs(ScoreDoc[] scoreDocs, boolean isSortedByField, SortField[] sortFields) { + this.scoreDocs = scoreDocs; + this.isSortedByField = isSortedByField; + this.sortFields = sortFields; + } + } } diff --git a/core/src/main/java/org/elasticsearch/action/search/SearchScrollQueryAndFetchAsyncAction.java b/core/src/main/java/org/elasticsearch/action/search/SearchScrollQueryAndFetchAsyncAction.java index c39a9fe6f25..b3ebaed3cb6 100644 --- a/core/src/main/java/org/elasticsearch/action/search/SearchScrollQueryAndFetchAsyncAction.java +++ b/core/src/main/java/org/elasticsearch/action/search/SearchScrollQueryAndFetchAsyncAction.java @@ -173,9 +173,8 @@ final class SearchScrollQueryAndFetchAsyncAction extends AbstractAsyncAction { private void innerFinishHim() throws Exception { List queryFetchSearchResults = queryFetchResults.asList(); - ScoreDoc[] sortedShardDocs = searchPhaseController.sortDocs(true, queryFetchResults.asList()); - final InternalSearchResponse internalResponse = searchPhaseController.merge(true, sortedShardDocs, - searchPhaseController.reducedQueryPhase(queryFetchSearchResults), queryFetchSearchResults, queryFetchResults::get); + final InternalSearchResponse internalResponse = searchPhaseController.merge(true, + searchPhaseController.reducedQueryPhase(queryFetchSearchResults, true), queryFetchSearchResults, queryFetchResults::get); String scrollId = null; if (request.scroll() != null) { scrollId = request.scrollId(); diff --git a/core/src/main/java/org/elasticsearch/action/search/SearchScrollQueryThenFetchAsyncAction.java b/core/src/main/java/org/elasticsearch/action/search/SearchScrollQueryThenFetchAsyncAction.java index 37071485a03..709738dcafb 100644 --- a/core/src/main/java/org/elasticsearch/action/search/SearchScrollQueryThenFetchAsyncAction.java +++ b/core/src/main/java/org/elasticsearch/action/search/SearchScrollQueryThenFetchAsyncAction.java @@ -55,7 +55,6 @@ final class SearchScrollQueryThenFetchAsyncAction extends AbstractAsyncAction { private volatile AtomicArray shardFailures; final AtomicArray queryResults; final AtomicArray fetchResults; - private volatile ScoreDoc[] sortedShardDocs; private final AtomicInteger successfulOps; SearchScrollQueryThenFetchAsyncAction(Logger logger, ClusterService clusterService, SearchTransportService searchTransportService, @@ -171,16 +170,15 @@ final class SearchScrollQueryThenFetchAsyncAction extends AbstractAsyncAction { } private void executeFetchPhase() throws Exception { - sortedShardDocs = searchPhaseController.sortDocs(true, queryResults.asList()); - if (sortedShardDocs.length == 0) { - finishHim(searchPhaseController.reducedQueryPhase(queryResults.asList())); + final SearchPhaseController.ReducedQueryPhase reducedQueryPhase = searchPhaseController.reducedQueryPhase(queryResults.asList(), + true); + if (reducedQueryPhase.scoreDocs.length == 0) { + finishHim(reducedQueryPhase); return; } - final IntArrayList[] docIdsToLoad = searchPhaseController.fillDocIdsToLoad(queryResults.length(), sortedShardDocs); - SearchPhaseController.ReducedQueryPhase reducedQueryPhase = searchPhaseController.reducedQueryPhase(queryResults.asList()); - final ScoreDoc[] lastEmittedDocPerShard = searchPhaseController.getLastEmittedDocPerShard(reducedQueryPhase, sortedShardDocs, - queryResults.length()); + final IntArrayList[] docIdsToLoad = searchPhaseController.fillDocIdsToLoad(queryResults.length(), reducedQueryPhase.scoreDocs); + final ScoreDoc[] lastEmittedDocPerShard = searchPhaseController.getLastEmittedDocPerShard(reducedQueryPhase, queryResults.length()); final CountDown counter = new CountDown(docIdsToLoad.length); for (int i = 0; i < docIdsToLoad.length; i++) { final int index = i; @@ -222,8 +220,8 @@ final class SearchScrollQueryThenFetchAsyncAction extends AbstractAsyncAction { private void finishHim(SearchPhaseController.ReducedQueryPhase queryPhase) { try { - final InternalSearchResponse internalResponse = searchPhaseController.merge(true, sortedShardDocs, queryPhase, - fetchResults.asList(), fetchResults::get); + final InternalSearchResponse internalResponse = searchPhaseController.merge(true, queryPhase, fetchResults.asList(), + fetchResults::get); String scrollId = null; if (request.scroll() != null) { scrollId = request.scrollId(); diff --git a/core/src/main/java/org/elasticsearch/common/util/concurrent/AtomicArray.java b/core/src/main/java/org/elasticsearch/common/util/concurrent/AtomicArray.java index 2bf5e50a1c2..fa82aa0ac63 100644 --- a/core/src/main/java/org/elasticsearch/common/util/concurrent/AtomicArray.java +++ b/core/src/main/java/org/elasticsearch/common/util/concurrent/AtomicArray.java @@ -31,14 +31,6 @@ import java.util.concurrent.atomic.AtomicReferenceArray; * to get the concrete values as a list using {@link #asList()}. */ public class AtomicArray { - - private static final AtomicArray EMPTY = new AtomicArray(0); - - @SuppressWarnings("unchecked") - public static E empty() { - return (E) EMPTY; - } - private final AtomicReferenceArray array; private volatile List nonNullList; @@ -53,7 +45,6 @@ public class AtomicArray { return array.length(); } - /** * Sets the element at position {@code i} to the given value. * diff --git a/core/src/main/java/org/elasticsearch/search/SearchService.java b/core/src/main/java/org/elasticsearch/search/SearchService.java index a0352281952..e601cec0fea 100644 --- a/core/src/main/java/org/elasticsearch/search/SearchService.java +++ b/core/src/main/java/org/elasticsearch/search/SearchService.java @@ -259,7 +259,7 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv loadOrExecuteQueryPhase(request, context); - if (context.queryResult().hasHits() == false && context.scrollContext() == null) { + if (context.queryResult().hasSearchContext() == false && context.scrollContext() == null) { freeContext(context.id()); } else { contextProcessedSuccessfully(context); @@ -341,7 +341,7 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv operationListener.onPreQueryPhase(context); long time = System.nanoTime(); queryPhase.execute(context); - if (context.queryResult().hasHits() == false && context.scrollContext() == null) { + if (context.queryResult().hasSearchContext() == false && context.scrollContext() == null) { // no hits, we can release the context since there will be no fetch phase freeContext(context.id()); } else { diff --git a/core/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java b/core/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java index 83af0b9abd4..97f2681252b 100644 --- a/core/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java +++ b/core/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java @@ -166,7 +166,7 @@ public class FetchPhase implements SearchPhase { fetchSubPhase.hitsExecute(context, hits); } - context.fetchResult().hits(new SearchHits(hits, context.queryResult().topDocs().totalHits, context.queryResult().topDocs().getMaxScore())); + context.fetchResult().hits(new SearchHits(hits, context.queryResult().getTotalHits(), context.queryResult().getMaxScore())); } private int findRootDocumentIfNested(SearchContext context, LeafReaderContext subReaderContext, int subDocId) throws IOException { diff --git a/core/src/main/java/org/elasticsearch/search/query/QueryPhase.java b/core/src/main/java/org/elasticsearch/search/query/QueryPhase.java index 13f32f74d0d..272c57fe980 100644 --- a/core/src/main/java/org/elasticsearch/search/query/QueryPhase.java +++ b/core/src/main/java/org/elasticsearch/search/query/QueryPhase.java @@ -142,7 +142,6 @@ public class QueryPhase implements SearchPhase { queryResult.searchTimedOut(false); final boolean doProfile = searchContext.getProfilers() != null; - final SearchType searchType = searchContext.searchType(); boolean rescore = false; try { queryResult.from(searchContext.from()); @@ -165,12 +164,7 @@ public class QueryPhase implements SearchPhase { if (searchContext.getProfilers() != null) { collector = new InternalProfileCollector(collector, CollectorResult.REASON_SEARCH_COUNT, Collections.emptyList()); } - topDocsCallable = new Callable() { - @Override - public TopDocs call() throws Exception { - return new TopDocs(totalHitCountCollector.getTotalHits(), Lucene.EMPTY_SCORE_DOCS, 0); - } - }; + topDocsCallable = () -> new TopDocs(totalHitCountCollector.getTotalHits(), Lucene.EMPTY_SCORE_DOCS, 0); } else { // Perhaps have a dedicated scroll phase? final ScrollContext scrollContext = searchContext.scrollContext(); @@ -238,38 +232,35 @@ public class QueryPhase implements SearchPhase { if (doProfile) { collector = new InternalProfileCollector(collector, CollectorResult.REASON_SEARCH_TOP_HITS, Collections.emptyList()); } - topDocsCallable = new Callable() { - @Override - public TopDocs call() throws Exception { - final TopDocs topDocs; - if (topDocsCollector instanceof TopDocsCollector) { - topDocs = ((TopDocsCollector) topDocsCollector).topDocs(); - } else if (topDocsCollector instanceof CollapsingTopDocsCollector) { - topDocs = ((CollapsingTopDocsCollector) topDocsCollector).getTopDocs(); - } else { - throw new IllegalStateException("Unknown top docs collector " + topDocsCollector.getClass().getName()); - } - if (scrollContext != null) { - if (scrollContext.totalHits == -1) { - // first round - scrollContext.totalHits = topDocs.totalHits; - scrollContext.maxScore = topDocs.getMaxScore(); - } else { - // subsequent round: the total number of hits and - // the maximum score were computed on the first round - topDocs.totalHits = scrollContext.totalHits; - topDocs.setMaxScore(scrollContext.maxScore); - } - if (searchContext.request().numberOfShards() == 1) { - // if we fetch the document in the same roundtrip, we already know the last emitted doc - if (topDocs.scoreDocs.length > 0) { - // set the last emitted doc - scrollContext.lastEmittedDoc = topDocs.scoreDocs[topDocs.scoreDocs.length - 1]; - } - } - } - return topDocs; + topDocsCallable = () -> { + final TopDocs topDocs; + if (topDocsCollector instanceof TopDocsCollector) { + topDocs = ((TopDocsCollector) topDocsCollector).topDocs(); + } else if (topDocsCollector instanceof CollapsingTopDocsCollector) { + topDocs = ((CollapsingTopDocsCollector) topDocsCollector).getTopDocs(); + } else { + throw new IllegalStateException("Unknown top docs collector " + topDocsCollector.getClass().getName()); } + if (scrollContext != null) { + if (scrollContext.totalHits == -1) { + // first round + scrollContext.totalHits = topDocs.totalHits; + scrollContext.maxScore = topDocs.getMaxScore(); + } else { + // subsequent round: the total number of hits and + // the maximum score were computed on the first round + topDocs.totalHits = scrollContext.totalHits; + topDocs.setMaxScore(scrollContext.maxScore); + } + if (searchContext.request().numberOfShards() == 1) { + // if we fetch the document in the same roundtrip, we already know the last emitted doc + if (topDocs.scoreDocs.length > 0) { + // set the last emitted doc + scrollContext.lastEmittedDoc = topDocs.scoreDocs[topDocs.scoreDocs.length - 1]; + } + } + } + return topDocs; }; } diff --git a/core/src/main/java/org/elasticsearch/search/query/QuerySearchResult.java b/core/src/main/java/org/elasticsearch/search/query/QuerySearchResult.java index 15403f99677..f071c62f12c 100644 --- a/core/src/main/java/org/elasticsearch/search/query/QuerySearchResult.java +++ b/core/src/main/java/org/elasticsearch/search/query/QuerySearchResult.java @@ -55,6 +55,9 @@ public final class QuerySearchResult extends SearchPhaseResult { private Boolean terminatedEarly = null; private ProfileShardResult profileShardResults; private boolean hasProfileResults; + private boolean hasScoreDocs; + private int totalHits; + private float maxScore; public QuerySearchResult() { } @@ -87,11 +90,34 @@ public final class QuerySearchResult extends SearchPhaseResult { } public TopDocs topDocs() { + if (topDocs == null) { + throw new IllegalStateException("topDocs already consumed"); + } + return topDocs; + } + + /** + * Returns true iff the top docs have already been consumed. + */ + public boolean hasConsumedTopDocs() { + return topDocs == null; + } + + /** + * Returns and nulls out the top docs for this search results. This allows to free up memory once the top docs are consumed. + * @throws IllegalStateException if the top docs have already been consumed. + */ + public TopDocs consumeTopDocs() { + TopDocs topDocs = this.topDocs; + if (topDocs == null) { + throw new IllegalStateException("topDocs already consumed"); + } + this.topDocs = null; return topDocs; } public void topDocs(TopDocs topDocs, DocValueFormat[] sortValueFormats) { - this.topDocs = topDocs; + setTopDocs(topDocs); if (topDocs.scoreDocs.length > 0 && topDocs.scoreDocs[0] instanceof FieldDoc) { int numFields = ((FieldDoc) topDocs.scoreDocs[0]).fields.length; if (numFields != sortValueFormats.length) { @@ -102,12 +128,19 @@ public final class QuerySearchResult extends SearchPhaseResult { this.sortValueFormats = sortValueFormats; } + private void setTopDocs(TopDocs topDocs) { + this.topDocs = topDocs; + hasScoreDocs = topDocs.scoreDocs.length > 0; + this.totalHits = topDocs.totalHits; + this.maxScore = topDocs.getMaxScore(); + } + public DocValueFormat[] sortValueFormats() { return sortValueFormats; } /** - * Retruns true if this query result has unconsumed aggregations + * Returns true if this query result has unconsumed aggregations */ public boolean hasAggs() { return hasAggs; @@ -195,10 +228,15 @@ public final class QuerySearchResult extends SearchPhaseResult { return this; } - /** Returns true iff the result has hits */ - public boolean hasHits() { - return (topDocs != null && topDocs.scoreDocs.length > 0) || - (suggest != null && suggest.hasScoreDocs()); + /** + * Returns true if this result has any suggest score docs + */ + public boolean hasSuggestHits() { + return (suggest != null && suggest.hasScoreDocs()); + } + + public boolean hasSearchContext() { + return hasScoreDocs || hasSuggestHits(); } public static QuerySearchResult readQuerySearchResult(StreamInput in) throws IOException { @@ -227,7 +265,7 @@ public final class QuerySearchResult extends SearchPhaseResult { sortValueFormats[i] = in.readNamedWriteable(DocValueFormat.class); } } - topDocs = readTopDocs(in); + setTopDocs(readTopDocs(in)); if (hasAggs = in.readBoolean()) { aggregations = InternalAggregations.readAggregations(in); } @@ -278,4 +316,12 @@ public final class QuerySearchResult extends SearchPhaseResult { out.writeOptionalBoolean(terminatedEarly); out.writeOptionalWriteable(profileShardResults); } + + public int getTotalHits() { + return totalHits; + } + + public float getMaxScore() { + return maxScore; + } } diff --git a/core/src/test/java/org/elasticsearch/action/search/SearchPhaseControllerTests.java b/core/src/test/java/org/elasticsearch/action/search/SearchPhaseControllerTests.java index 207183bae4e..c92caef628a 100644 --- a/core/src/test/java/org/elasticsearch/action/search/SearchPhaseControllerTests.java +++ b/core/src/test/java/org/elasticsearch/action/search/SearchPhaseControllerTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.action.search; +import com.carrotsearch.randomizedtesting.RandomizedContext; import org.apache.lucene.search.ScoreDoc; import org.apache.lucene.search.TopDocs; import org.elasticsearch.common.lucene.Lucene; @@ -42,6 +43,7 @@ import org.elasticsearch.search.query.QuerySearchResult; import org.elasticsearch.search.suggest.Suggest; import org.elasticsearch.search.suggest.completion.CompletionSuggestion; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.TestCluster; import org.junit.Before; import java.io.IOException; @@ -51,12 +53,16 @@ import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Optional; +import java.util.concurrent.Callable; import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Collectors; +import java.util.stream.Stream; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.lessThanOrEqualTo; import static org.hamcrest.Matchers.not; public class SearchPhaseControllerTests extends ESTestCase { @@ -75,8 +81,16 @@ public class SearchPhaseControllerTests extends ESTestCase { int nShards = randomIntBetween(1, 20); int queryResultSize = randomBoolean() ? 0 : randomIntBetween(1, nShards * 2); AtomicArray results = generateQueryResults(nShards, suggestions, queryResultSize, false); - ScoreDoc[] sortedDocs = searchPhaseController.sortDocs(true, results.asList()); + Optional first = results.asList().stream().findFirst(); + int from = 0, size = 0; + if (first.isPresent()) { + from = first.get().queryResult().from(); + size = first.get().queryResult().size(); + } int accumulatedLength = Math.min(queryResultSize, getTotalQueryHits(results)); + ScoreDoc[] sortedDocs = searchPhaseController.sortDocs(true, results.asList(), null, new SearchPhaseController.TopDocsStats(), + from, size) + .scoreDocs; for (Suggest.Suggestion suggestion : reducedSuggest(results)) { int suggestionSize = suggestion.getEntries().get(0).getOptions().size(); accumulatedLength += suggestionSize; @@ -84,48 +98,71 @@ public class SearchPhaseControllerTests extends ESTestCase { assertThat(sortedDocs.length, equalTo(accumulatedLength)); } - public void testSortIsIdempotent() throws IOException { + public void testSortIsIdempotent() throws Exception { int nShards = randomIntBetween(1, 20); int queryResultSize = randomBoolean() ? 0 : randomIntBetween(1, nShards * 2); - AtomicArray results = generateQueryResults(nShards, Collections.emptyList(), queryResultSize, - randomBoolean() || true); + long randomSeed = randomLong(); + boolean useConstantScore = randomBoolean(); + AtomicArray results = generateSeededQueryResults(randomSeed, nShards, Collections.emptyList(), queryResultSize, + useConstantScore); boolean ignoreFrom = randomBoolean(); - ScoreDoc[] sortedDocs = searchPhaseController.sortDocs(ignoreFrom, results.asList()); + Optional first = results.asList().stream().findFirst(); + int from = 0, size = 0; + if (first.isPresent()) { + from = first.get().queryResult().from(); + size = first.get().queryResult().size(); + } + SearchPhaseController.TopDocsStats topDocsStats = new SearchPhaseController.TopDocsStats(); + ScoreDoc[] sortedDocs = searchPhaseController.sortDocs(ignoreFrom, results.asList(), null, topDocsStats, from, size).scoreDocs; - ScoreDoc[] sortedDocs2 = searchPhaseController.sortDocs(ignoreFrom, results.asList()); - assertArrayEquals(sortedDocs, sortedDocs2); + results = generateSeededQueryResults(randomSeed, nShards, Collections.emptyList(), queryResultSize, + useConstantScore); + SearchPhaseController.TopDocsStats topDocsStats2 = new SearchPhaseController.TopDocsStats(); + ScoreDoc[] sortedDocs2 = searchPhaseController.sortDocs(ignoreFrom, results.asList(), null, topDocsStats2, from, size).scoreDocs; + assertEquals(sortedDocs.length, sortedDocs2.length); + for (int i = 0; i < sortedDocs.length; i++) { + assertEquals(sortedDocs[i].doc, sortedDocs2[i].doc); + assertEquals(sortedDocs[i].shardIndex, sortedDocs2[i].shardIndex); + assertEquals(sortedDocs[i].score, sortedDocs2[i].score, 0.0f); + } + assertEquals(topDocsStats.maxScore, topDocsStats2.maxScore, 0.0f); + assertEquals(topDocsStats.totalHits, topDocsStats2.totalHits); + assertEquals(topDocsStats.fetchHits, topDocsStats2.fetchHits); + } + + private AtomicArray generateSeededQueryResults(long seed, int nShards, + List suggestions, + int searchHitsSize, boolean useConstantScore) throws Exception { + return RandomizedContext.current().runWithPrivateRandomness(seed, + () -> generateQueryResults(nShards, suggestions, searchHitsSize, useConstantScore)); } public void testMerge() throws IOException { List suggestions = new ArrayList<>(); + int maxSuggestSize = 0; for (int i = 0; i < randomIntBetween(1, 5); i++) { - suggestions.add(new CompletionSuggestion(randomAlphaOfLength(randomIntBetween(1, 5)), randomIntBetween(1, 20))); + int size = randomIntBetween(1, 20); + maxSuggestSize += size; + suggestions.add(new CompletionSuggestion(randomAlphaOfLength(randomIntBetween(1, 5)), size)); } int nShards = randomIntBetween(1, 20); int queryResultSize = randomBoolean() ? 0 : randomIntBetween(1, nShards * 2); AtomicArray queryResults = generateQueryResults(nShards, suggestions, queryResultSize, false); - - // calculate offsets and score doc array - List mergedScoreDocs = new ArrayList<>(); - ScoreDoc[] mergedSearchDocs = getTopShardDocs(queryResults); - mergedScoreDocs.addAll(Arrays.asList(mergedSearchDocs)); - Suggest mergedSuggest = reducedSuggest(queryResults); - for (Suggest.Suggestion suggestion : mergedSuggest) { - if (suggestion instanceof CompletionSuggestion) { - CompletionSuggestion completionSuggestion = ((CompletionSuggestion) suggestion); - mergedScoreDocs.addAll(completionSuggestion.getOptions().stream() - .map(CompletionSuggestion.Entry.Option::getDoc) - .collect(Collectors.toList())); - } - } - ScoreDoc[] sortedDocs = mergedScoreDocs.toArray(new ScoreDoc[mergedScoreDocs.size()]); - AtomicArray searchPhaseResultAtomicArray = generateFetchResults(nShards, mergedSearchDocs, mergedSuggest); - InternalSearchResponse mergedResponse = searchPhaseController.merge(true, sortedDocs, - searchPhaseController.reducedQueryPhase(queryResults.asList()), + SearchPhaseController.ReducedQueryPhase reducedQueryPhase = searchPhaseController.reducedQueryPhase(queryResults.asList(), false); + AtomicArray searchPhaseResultAtomicArray = generateFetchResults(nShards, reducedQueryPhase.scoreDocs, + reducedQueryPhase.suggest); + InternalSearchResponse mergedResponse = searchPhaseController.merge(false, + reducedQueryPhase, searchPhaseResultAtomicArray.asList(), searchPhaseResultAtomicArray::get); - assertThat(mergedResponse.hits().getHits().length, equalTo(mergedSearchDocs.length)); + int suggestSize = 0; + for (Suggest.Suggestion s : reducedQueryPhase.suggest) { + Stream stream = s.getEntries().stream(); + suggestSize += stream.collect(Collectors.summingInt(e -> e.getOptions().size())); + } + assertThat(suggestSize, lessThanOrEqualTo(maxSuggestSize)); + assertThat(mergedResponse.hits().getHits().length, equalTo(reducedQueryPhase.scoreDocs.length-suggestSize)); Suggest suggestResult = mergedResponse.suggest(); - for (Suggest.Suggestion suggestion : mergedSuggest) { + for (Suggest.Suggestion suggestion : reducedQueryPhase.suggest) { assertThat(suggestion, instanceOf(CompletionSuggestion.class)); if (suggestion.getEntries().get(0).getOptions().size() > 0) { CompletionSuggestion suggestionResult = suggestResult.getSuggestion(suggestion.getName()); @@ -209,16 +246,6 @@ public class SearchPhaseControllerTests extends ESTestCase { .collect(Collectors.toList())); } - private ScoreDoc[] getTopShardDocs(AtomicArray results) throws IOException { - List resultList = results.asList(); - TopDocs[] shardTopDocs = new TopDocs[resultList.size()]; - for (int i = 0; i < resultList.size(); i++) { - shardTopDocs[i] = resultList.get(i).queryResult().topDocs(); - } - int topN = Math.min(results.get(0).queryResult().size(), getTotalQueryHits(results)); - return TopDocs.merge(topN, shardTopDocs).scoreDocs; - } - private AtomicArray generateFetchResults(int nShards, ScoreDoc[] mergedSearchDocs, Suggest mergedSuggest) { AtomicArray fetchResults = new AtomicArray<>(nShards); for (int shardIndex = 0; shardIndex < nShards; shardIndex++) { @@ -309,30 +336,96 @@ public class SearchPhaseControllerTests extends ESTestCase { InitialSearchPhase.SearchPhaseResults consumer = searchPhaseController.newSearchPhaseResults(request, expectedNumResults); AtomicInteger max = new AtomicInteger(); - CountDownLatch latch = new CountDownLatch(expectedNumResults); + Thread[] threads = new Thread[expectedNumResults]; for (int i = 0; i < expectedNumResults; i++) { int id = i; - Thread t = new Thread(() -> { + threads[i] = new Thread(() -> { int number = randomIntBetween(1, 1000); max.updateAndGet(prev -> Math.max(prev, number)); QuerySearchResult result = new QuerySearchResult(id, new SearchShardTarget("node", new Index("a", "b"), id)); - result.topDocs(new TopDocs(id, new ScoreDoc[0], 0.0F), new DocValueFormat[0]); + result.topDocs(new TopDocs(1, new ScoreDoc[] {new ScoreDoc(0, number)}, number), new DocValueFormat[0]); InternalAggregations aggs = new InternalAggregations(Arrays.asList(new InternalMax("test", (double) number, DocValueFormat.RAW, Collections.emptyList(), Collections.emptyMap()))); result.aggregations(aggs); result.setShardIndex(id); + result.size(1); consumer.consumeResult(result); - latch.countDown(); }); - t.start(); + threads[i].start(); + } + for (int i = 0; i < expectedNumResults; i++) { + threads[i].join(); } - latch.await(); SearchPhaseController.ReducedQueryPhase reduce = consumer.reduce(); InternalMax internalMax = (InternalMax) reduce.aggregations.asList().get(0); assertEquals(max.get(), internalMax.getValue(), 0.0D); + assertEquals(1, reduce.scoreDocs.length); + assertEquals(max.get(), reduce.maxScore, 0.0f); + assertEquals(expectedNumResults, reduce.totalHits); + assertEquals(max.get(), reduce.scoreDocs[0].score, 0.0f); } + public void testConsumerOnlyAggs() throws InterruptedException { + int expectedNumResults = randomIntBetween(1, 100); + int bufferSize = randomIntBetween(2, 200); + SearchRequest request = new SearchRequest(); + request.source(new SearchSourceBuilder().aggregation(AggregationBuilders.avg("foo")).size(0)); + request.setBatchedReduceSize(bufferSize); + InitialSearchPhase.SearchPhaseResults consumer = + searchPhaseController.newSearchPhaseResults(request, expectedNumResults); + AtomicInteger max = new AtomicInteger(); + for (int i = 0; i < expectedNumResults; i++) { + int id = i; + int number = randomIntBetween(1, 1000); + max.updateAndGet(prev -> Math.max(prev, number)); + QuerySearchResult result = new QuerySearchResult(id, new SearchShardTarget("node", new Index("a", "b"), id)); + result.topDocs(new TopDocs(1, new ScoreDoc[0], number), new DocValueFormat[0]); + InternalAggregations aggs = new InternalAggregations(Arrays.asList(new InternalMax("test", (double) number, + DocValueFormat.RAW, Collections.emptyList(), Collections.emptyMap()))); + result.aggregations(aggs); + result.setShardIndex(id); + result.size(1); + consumer.consumeResult(result); + } + SearchPhaseController.ReducedQueryPhase reduce = consumer.reduce(); + InternalMax internalMax = (InternalMax) reduce.aggregations.asList().get(0); + assertEquals(max.get(), internalMax.getValue(), 0.0D); + assertEquals(0, reduce.scoreDocs.length); + assertEquals(max.get(), reduce.maxScore, 0.0f); + assertEquals(expectedNumResults, reduce.totalHits); + } + + + public void testConsumerOnlyHits() throws InterruptedException { + int expectedNumResults = randomIntBetween(1, 100); + int bufferSize = randomIntBetween(2, 200); + SearchRequest request = new SearchRequest(); + if (randomBoolean()) { + request.source(new SearchSourceBuilder().size(randomIntBetween(1, 10))); + } + request.setBatchedReduceSize(bufferSize); + InitialSearchPhase.SearchPhaseResults consumer = + searchPhaseController.newSearchPhaseResults(request, expectedNumResults); + AtomicInteger max = new AtomicInteger(); + for (int i = 0; i < expectedNumResults; i++) { + int id = i; + int number = randomIntBetween(1, 1000); + max.updateAndGet(prev -> Math.max(prev, number)); + QuerySearchResult result = new QuerySearchResult(id, new SearchShardTarget("node", new Index("a", "b"), id)); + result.topDocs(new TopDocs(1, new ScoreDoc[] {new ScoreDoc(0, number)}, number), new DocValueFormat[0]); + result.setShardIndex(id); + result.size(1); + consumer.consumeResult(result); + } + SearchPhaseController.ReducedQueryPhase reduce = consumer.reduce(); + assertEquals(1, reduce.scoreDocs.length); + assertEquals(max.get(), reduce.maxScore, 0.0f); + assertEquals(expectedNumResults, reduce.totalHits); + assertEquals(max.get(), reduce.scoreDocs[0].score, 0.0f); + } + + public void testNewSearchPhaseResults() { for (int i = 0; i < 10; i++) { int expectedNumResults = randomIntBetween(1, 10); @@ -342,10 +435,22 @@ public class SearchPhaseControllerTests extends ESTestCase { if ((hasAggs = randomBoolean())) { request.source(new SearchSourceBuilder().aggregation(AggregationBuilders.avg("foo"))); } + final boolean hasTopDocs; + if ((hasTopDocs = randomBoolean())) { + if (request.source() != null) { + request.source().size(randomIntBetween(1, 100)); + } // no source means size = 10 + } else { + if (request.source() == null) { + request.source(new SearchSourceBuilder().size(0)); + } else { + request.source().size(0); + } + } request.setBatchedReduceSize(bufferSize); InitialSearchPhase.SearchPhaseResults consumer = searchPhaseController.newSearchPhaseResults(request, expectedNumResults); - if (hasAggs && expectedNumResults > bufferSize) { + if ((hasAggs || hasTopDocs) && expectedNumResults > bufferSize) { assertThat("expectedNumResults: " + expectedNumResults + " bufferSize: " + bufferSize, consumer, instanceOf(SearchPhaseController.QueryPhaseResultConsumer.class)); } else { @@ -354,4 +459,36 @@ public class SearchPhaseControllerTests extends ESTestCase { } } } + + public void testReduceTopNWithFromOffset() { + SearchRequest request = new SearchRequest(); + request.source(new SearchSourceBuilder().size(5).from(5)); + request.setBatchedReduceSize(randomIntBetween(2, 4)); + InitialSearchPhase.SearchPhaseResults consumer = + searchPhaseController.newSearchPhaseResults(request, 4); + int score = 100; + for (int i = 0; i < 4; i++) { + QuerySearchResult result = new QuerySearchResult(i, new SearchShardTarget("node", new Index("a", "b"), i)); + ScoreDoc[] docs = new ScoreDoc[3]; + for (int j = 0; j < docs.length; j++) { + docs[j] = new ScoreDoc(0, score--); + } + result.topDocs(new TopDocs(3, docs, docs[0].score), new DocValueFormat[0]); + result.setShardIndex(i); + result.size(5); + result.from(5); + consumer.consumeResult(result); + } + // 4*3 results = 12 we get result 5 to 10 here with from=5 and size=5 + + SearchPhaseController.ReducedQueryPhase reduce = consumer.reduce(); + assertEquals(5, reduce.scoreDocs.length); + assertEquals(100.f, reduce.maxScore, 0.0f); + assertEquals(12, reduce.totalHits); + assertEquals(95.0f, reduce.scoreDocs[0].score, 0.0f); + assertEquals(94.0f, reduce.scoreDocs[1].score, 0.0f); + assertEquals(93.0f, reduce.scoreDocs[2].score, 0.0f); + assertEquals(92.0f, reduce.scoreDocs[3].score, 0.0f); + assertEquals(91.0f, reduce.scoreDocs[4].score, 0.0f); + } } diff --git a/core/src/test/java/org/elasticsearch/search/SearchServiceTests.java b/core/src/test/java/org/elasticsearch/search/SearchServiceTests.java index f3ff6be1cc1..6fc795a8825 100644 --- a/core/src/test/java/org/elasticsearch/search/SearchServiceTests.java +++ b/core/src/test/java/org/elasticsearch/search/SearchServiceTests.java @@ -223,8 +223,13 @@ public class SearchServiceTests extends ESSingleNodeTestCase { new AliasFilter(null, Strings.EMPTY_ARRAY), 1.0f), null); - // the search context should inherit the default timeout - assertThat(contextWithDefaultTimeout.timeout(), equalTo(TimeValue.timeValueSeconds(5))); + try { + // the search context should inherit the default timeout + assertThat(contextWithDefaultTimeout.timeout(), equalTo(TimeValue.timeValueSeconds(5))); + } finally { + contextWithDefaultTimeout.decRef(); + service.freeContext(contextWithDefaultTimeout.id()); + } final long seconds = randomIntBetween(6, 10); final SearchContext context = service.createContext( @@ -238,8 +243,14 @@ public class SearchServiceTests extends ESSingleNodeTestCase { new AliasFilter(null, Strings.EMPTY_ARRAY), 1.0f), null); - // the search context should inherit the query timeout - assertThat(context.timeout(), equalTo(TimeValue.timeValueSeconds(seconds))); + try { + // the search context should inherit the query timeout + assertThat(context.timeout(), equalTo(TimeValue.timeValueSeconds(seconds))); + } finally { + context.decRef(); + service.freeContext(context.id()); + } + } public static class FailOnRewriteQueryPlugin extends Plugin implements SearchPlugin { From 9b3c85dd88367355d5984bb946711ee295946265 Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Mon, 10 Apr 2017 10:10:16 +0200 Subject: [PATCH 17/26] Deprecate _field_stats endpoint (#23914) _field_stats has evolved quite a lot to become a multi purpose API capable of retrieving the field capabilities and the min/max value for a field. In the mean time a more focused API called `_field_caps` has been added, this enpoint is a good replacement for _field_stats since he can retrieve the field capabilities by just looking at the field mapping (no lookup in the index structures). Also the recent improvement made to range queries makes the _field_stats API obsolete since this queries are now rewritten per shard based on the min/max found for the field. This means that a range query that does not match any document in a shard can return quickly and can be cached efficiently. For these reasons this change deprecates _field_stats. The deprecation should happen in 5.4 but we won't remove this API in 6.x yet which is why this PR is made directly to 6.0. The rest tests have also been adapted to not throw an error while this change is backported to 5.4. --- .../java/org/elasticsearch/client/Client.java | 12 ++++ .../rest/action/RestFieldStatsAction.java | 17 ++++-- docs/reference/search/field-stats.asciidoc | 9 +++ .../test/field_caps/10_basic.yaml | 12 ++-- .../test/field_stats/10_basics.yaml | 55 +++++++++++++++++-- 5 files changed, 89 insertions(+), 16 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/client/Client.java b/core/src/main/java/org/elasticsearch/client/Client.java index 663b820dc39..2080856e302 100644 --- a/core/src/main/java/org/elasticsearch/client/Client.java +++ b/core/src/main/java/org/elasticsearch/client/Client.java @@ -456,10 +456,22 @@ public interface Client extends ElasticsearchClient, Releasable { */ void clearScroll(ClearScrollRequest request, ActionListener listener); + /** + * @deprecated Use _field_caps instead or run a min/max aggregations on the desired fields + */ + @Deprecated FieldStatsRequestBuilder prepareFieldStats(); + /** + * @deprecated Use _field_caps instead or run a min/max aggregations on the desired fields + */ + @Deprecated ActionFuture fieldStats(FieldStatsRequest request); + /** + * @deprecated Use _field_caps instead or run a min/max aggregations on the desired fields + */ + @Deprecated void fieldStats(FieldStatsRequest request, ActionListener listener); /** diff --git a/core/src/main/java/org/elasticsearch/rest/action/RestFieldStatsAction.java b/core/src/main/java/org/elasticsearch/rest/action/RestFieldStatsAction.java index 1393f429372..34e7b636aeb 100644 --- a/core/src/main/java/org/elasticsearch/rest/action/RestFieldStatsAction.java +++ b/core/src/main/java/org/elasticsearch/rest/action/RestFieldStatsAction.java @@ -45,10 +45,19 @@ import static org.elasticsearch.rest.action.RestActions.buildBroadcastShardsHead public class RestFieldStatsAction extends BaseRestHandler { public RestFieldStatsAction(Settings settings, RestController controller) { super(settings); - controller.registerHandler(GET, "/_field_stats", this); - controller.registerHandler(POST, "/_field_stats", this); - controller.registerHandler(GET, "/{index}/_field_stats", this); - controller.registerHandler(POST, "/{index}/_field_stats", this); + controller.registerAsDeprecatedHandler(GET, "/_field_stats", this, + deprecationMessage(), deprecationLogger); + controller.registerAsDeprecatedHandler(POST, "/_field_stats", this, + deprecationMessage(), deprecationLogger); + controller.registerAsDeprecatedHandler(GET, "/{index}/_field_stats", this, + deprecationMessage(), deprecationLogger); + controller.registerAsDeprecatedHandler(POST, "/{index}/_field_stats", this, + deprecationMessage(), deprecationLogger); + } + + static String deprecationMessage() { + return "[_field_stats] endpoint is deprecated! Use [_field_caps] instead or " + + "run a min/max aggregations on the desired fields."; } @Override diff --git a/docs/reference/search/field-stats.asciidoc b/docs/reference/search/field-stats.asciidoc index 78ce9e41f8f..2b4a4865129 100644 --- a/docs/reference/search/field-stats.asciidoc +++ b/docs/reference/search/field-stats.asciidoc @@ -3,6 +3,8 @@ experimental[] +deprecated[5.4.0, `_field_stats` is deprecated, use `_field_caps` instead or run an min/max aggregation on the desired fields] + The field stats api allows one to find statistical properties of a field without executing a search, but looking up measurements that are natively available in the Lucene index. This can be useful to explore a dataset which @@ -19,6 +21,7 @@ All indices: GET _field_stats?fields=rating -------------------------------------------------- // CONSOLE +// TEST[warning:[_field_stats] endpoint is deprecated! Use [_field_caps] instead or run a min/max aggregations on the desired fields.] Specific indices: @@ -27,6 +30,7 @@ Specific indices: GET twitter/_field_stats?fields=rating -------------------------------------------------- // CONSOLE +// TEST[warning:[_field_stats] endpoint is deprecated! Use [_field_caps] instead or run a min/max aggregations on the desired fields.] // TEST[setup:twitter] Supported request options: @@ -47,6 +51,7 @@ POST _field_stats?level=indices } -------------------------------------------------- // CONSOLE +// TEST[warning:[_field_stats] endpoint is deprecated! Use [_field_caps] instead or run a min/max aggregations on the desired fields.] This is equivalent to the previous request. @@ -122,6 +127,7 @@ Request: GET _field_stats?fields=rating,answer_count,creation_date,display_name -------------------------------------------------- // CONSOLE +// TEST[warning:[_field_stats] endpoint is deprecated! Use [_field_caps] instead or run a min/max aggregations on the desired fields.] Response: @@ -235,6 +241,7 @@ Request: GET _field_stats?fields=rating,answer_count,creation_date,display_name&level=indices -------------------------------------------------- // CONSOLE +// TEST[warning:[_field_stats] endpoint is deprecated! Use [_field_caps] instead or run a min/max aggregations on the desired fields.] Response: @@ -330,6 +337,7 @@ POST _field_stats?level=indices } -------------------------------------------------- // CONSOLE +// TEST[warning:[_field_stats] endpoint is deprecated! Use [_field_caps] instead or run a min/max aggregations on the desired fields.] <1> The fields to compute and return field stats for. <2> The set index constraints. Note that index constrains can be defined for fields that aren't defined in the `fields` option. @@ -368,5 +376,6 @@ POST _field_stats?level=indices } -------------------------------------------------- // CONSOLE +// TEST[warning:[_field_stats] endpoint is deprecated! Use [_field_caps] instead or run a min/max aggregations on the desired fields.] <1> Custom date format diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/field_caps/10_basic.yaml b/rest-api-spec/src/main/resources/rest-api-spec/test/field_caps/10_basic.yaml index edda7b6dbf3..cef72b6e3fe 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/field_caps/10_basic.yaml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/field_caps/10_basic.yaml @@ -76,8 +76,8 @@ setup: --- "Get simple field caps": - skip: - version: " - 5.99.99" - reason: this uses a new API that has been added in 6.0 + version: " - 5.3.99" + reason: this uses a new API that has been added in 5.4.0 - do: field_caps: @@ -117,8 +117,8 @@ setup: --- "Get nested field caps": - skip: - version: " - 5.99.99" - reason: this uses a new API that has been added in 6.0 + version: " - 5.3.99" + reason: this uses a new API that has been added in 5.4.0 - do: field_caps: @@ -148,8 +148,8 @@ setup: --- "Get prefix field caps": - skip: - version: " - 5.99.99" - reason: this uses a new API that has been added in 6.0 + version: " - 5.3.99" + reason: this uses a new API that has been added in 5.4.0 - do: field_caps: diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/field_stats/10_basics.yaml b/rest-api-spec/src/main/resources/rest-api-spec/test/field_stats/10_basics.yaml index 4e6cbd61623..907432d2d66 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/field_stats/10_basics.yaml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/field_stats/10_basics.yaml @@ -59,7 +59,14 @@ setup: --- "Basic field stats": + - skip: + version: " - 5.99.99" + reason: Deprecation was added in 6.0.0 + features: warnings + - do: + warnings: + - "[_field_stats] endpoint is deprecated! Use [_field_caps] instead or run a min/max aggregations on the desired fields." field_stats: fields: [foo, number] @@ -87,10 +94,13 @@ setup: --- "Geo field stats": - skip: - version: " - 5.3.0" - reason: geo_point fields don't return min/max for versions greater than 5.2.0 + version: " - 5.99.99" + reason: Deprecation was added in 6.0.0 + features: warnings - do: + warnings: + - "[_field_stats] endpoint is deprecated! Use [_field_caps] instead or run a min/max aggregations on the desired fields." field_stats: fields: [geo, geo_shape] @@ -116,7 +126,14 @@ setup: --- "Basic field stats with level set to indices": + - skip: + version: " - 5.99.99" + reason: Deprecation was added in 6.0.0 + features: warnings + - do: + warnings: + - "[_field_stats] endpoint is deprecated! Use [_field_caps] instead or run a min/max aggregations on the desired fields." field_stats: fields: [foo, number] level: indices @@ -164,10 +181,13 @@ setup: --- "Geo field stats with level set to indices": - skip: - version: " - 5.3.0" - reason: geo_point fields don't return min/max for versions greater than 5.2.0 + version: " - 5.99.99" + reason: Deprecation was added in 6.0.0 + features: warnings - do: + warnings: + - "[_field_stats] endpoint is deprecated! Use [_field_caps] instead or run a min/max aggregations on the desired fields." field_stats: fields: [geo, geo_shape] level: indices @@ -196,10 +216,13 @@ setup: --- "Geopoint field stats": - skip: - version: " - 5.3.0" - reason: geo_point type not handled for versions earlier than 6.0.0 + version: " - 5.99.99" + reason: Deprecation was added in 6.0.0 + features: warnings - do: + warnings: + - "[_field_stats] endpoint is deprecated! Use [_field_caps] instead or run a min/max aggregations on the desired fields." field_stats: fields: [geo] level: indices @@ -216,8 +239,14 @@ setup: --- "Field stats with filtering": + - skip: + version: " - 5.99.99" + reason: Deprecation was added in 6.0.0 + features: warnings - do: + warnings: + - "[_field_stats] endpoint is deprecated! Use [_field_caps] instead or run a min/max aggregations on the desired fields." field_stats: level: indices index: test_1 @@ -234,6 +263,8 @@ setup: - is_false: conflicts - do: + warnings: + - "[_field_stats] endpoint is deprecated! Use [_field_caps] instead or run a min/max aggregations on the desired fields." field_stats: level: indices index: test_1 @@ -243,6 +274,11 @@ setup: --- "Field stats both source and fields": + - skip: + version: " - 5.99.99" + reason: Deprecation was added in 6.0.0 + features: warnings + - do: catch: request field_stats: @@ -252,7 +288,14 @@ setup: --- "Field stats with conflicts": + - skip: + version: " - 5.99.99" + reason: Deprecation was added in 6.0.0 + features: warnings + - do: + warnings: + - "[_field_stats] endpoint is deprecated! Use [_field_caps] instead or run a min/max aggregations on the desired fields." field_stats: fields: [foo, number, bar] From 3b7bc8012a5a6630a18cbe93f00449efc5da4412 Mon Sep 17 00:00:00 2001 From: javanna Date: Mon, 10 Apr 2017 11:32:14 +0200 Subject: [PATCH 18/26] [TEST] increase minimum length of randomly generated fields in RandomObjects We had a couple of unfortunate field name collisions in our CI, where the json duplicate check tripped. Increasing the minimum length of randomly generated field names should decrease the chance of this issue happening again. --- .../src/main/java/org/elasticsearch/test/RandomObjects.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/framework/src/main/java/org/elasticsearch/test/RandomObjects.java b/test/framework/src/main/java/org/elasticsearch/test/RandomObjects.java index c9631034363..67999b82a2f 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/RandomObjects.java +++ b/test/framework/src/main/java/org/elasticsearch/test/RandomObjects.java @@ -190,11 +190,11 @@ public final class RandomObjects { for (int i = 0; i < numFields; i++) { if (currentDepth < 5 && random.nextBoolean()) { if (random.nextBoolean()) { - builder.startObject(RandomStrings.randomAsciiOfLengthBetween(random, 4, 10)); + builder.startObject(RandomStrings.randomAsciiOfLengthBetween(random, 6, 10)); addFields(random, builder, minNumFields, currentDepth + 1); builder.endObject(); } else { - builder.startArray(RandomStrings.randomAsciiOfLengthBetween(random, 4, 10)); + builder.startArray(RandomStrings.randomAsciiOfLengthBetween(random, 6, 10)); int numElements = randomIntBetween(random, 1, 5); boolean object = random.nextBoolean(); int dataType = -1; @@ -213,7 +213,7 @@ public final class RandomObjects { builder.endArray(); } } else { - builder.field(RandomStrings.randomAsciiOfLengthBetween(random, 4, 10), + builder.field(RandomStrings.randomAsciiOfLengthBetween(random, 6, 10), randomFieldValue(random, randomDataType(random))); } } From 12471c4f76636f837df363e55367c2fc5837ea75 Mon Sep 17 00:00:00 2001 From: Yannick Welsch Date: Mon, 10 Apr 2017 11:38:56 +0200 Subject: [PATCH 19/26] [TEST] Fix wait condition on testMultipleNodesShutdownNonMasterNodes After two nodes are being stopped and two more are joining the cluster, we first have to wait on the cluster to consist of the right nodes before waiting on green status, otherwise we might get a green status for a cluster with dead nodes. --- .../java/org/elasticsearch/cluster/MinimumMasterNodesIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/test/java/org/elasticsearch/cluster/MinimumMasterNodesIT.java b/core/src/test/java/org/elasticsearch/cluster/MinimumMasterNodesIT.java index 71fe7caabcf..c9057f4373f 100644 --- a/core/src/test/java/org/elasticsearch/cluster/MinimumMasterNodesIT.java +++ b/core/src/test/java/org/elasticsearch/cluster/MinimumMasterNodesIT.java @@ -248,8 +248,8 @@ public class MinimumMasterNodesIT extends ESIntegTestCase { logger.info("--> start back the 2 nodes "); String[] newNodes = internalCluster().startNodes(2, settings).stream().toArray(String[]::new); - ensureGreen(); internalCluster().validateClusterFormed(); + ensureGreen(); state = client().admin().cluster().prepareState().execute().actionGet().getState(); assertThat(state.nodes().getSize(), equalTo(4)); From b73f87b0ea4ec4a66717b30a671c7f4f379a3ca9 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Mon, 10 Apr 2017 12:27:42 +0200 Subject: [PATCH 20/26] Make buffer limit configurable in HeapBufferedConsumerFactory (#23970) The buffer limit should have been configurable already, but the factory constructor is package private so it is truly configurable only from the org.elasticsearch.client package. Also the HttpAsyncResponseConsumerFactory interface was package private, so it could only be implemented from the org.elasticsearch.client package. Closes #23958 --- .../resources/checkstyle_suppressions.xml | 1 + .../HttpAsyncResponseConsumerFactory.java | 4 +-- ...eapBufferedAsyncResponseConsumerTests.java | 27 ++++++++++++++++++- 3 files changed, 29 insertions(+), 3 deletions(-) diff --git a/buildSrc/src/main/resources/checkstyle_suppressions.xml b/buildSrc/src/main/resources/checkstyle_suppressions.xml index f50d0422380..8c5aa12739e 100644 --- a/buildSrc/src/main/resources/checkstyle_suppressions.xml +++ b/buildSrc/src/main/resources/checkstyle_suppressions.xml @@ -42,6 +42,7 @@ + diff --git a/client/rest/src/main/java/org/elasticsearch/client/HttpAsyncResponseConsumerFactory.java b/client/rest/src/main/java/org/elasticsearch/client/HttpAsyncResponseConsumerFactory.java index 528fb9a7fc8..1af9e0dcf0f 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/HttpAsyncResponseConsumerFactory.java +++ b/client/rest/src/main/java/org/elasticsearch/client/HttpAsyncResponseConsumerFactory.java @@ -29,7 +29,7 @@ import static org.elasticsearch.client.HttpAsyncResponseConsumerFactory.HeapBuff * consumer object. Users can implement this interface and pass their own instance to the specialized * performRequest methods that accept an {@link HttpAsyncResponseConsumerFactory} instance as argument. */ -interface HttpAsyncResponseConsumerFactory { +public interface HttpAsyncResponseConsumerFactory { /** * Creates the default type of {@link HttpAsyncResponseConsumer}, based on heap buffering with a buffer limit of 100MB. @@ -53,7 +53,7 @@ interface HttpAsyncResponseConsumerFactory { private final int bufferLimit; - HeapBufferedResponseConsumerFactory(int bufferLimitBytes) { + public HeapBufferedResponseConsumerFactory(int bufferLimitBytes) { this.bufferLimit = bufferLimitBytes; } diff --git a/client/rest/src/test/java/org/elasticsearch/client/HeapBufferedAsyncResponseConsumerTests.java b/client/rest/src/test/java/org/elasticsearch/client/HeapBufferedAsyncResponseConsumerTests.java index 85b7090bb94..fe82d5367e5 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/HeapBufferedAsyncResponseConsumerTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/HeapBufferedAsyncResponseConsumerTests.java @@ -24,19 +24,24 @@ import org.apache.http.HttpEntity; import org.apache.http.HttpResponse; import org.apache.http.ProtocolVersion; import org.apache.http.StatusLine; -import org.apache.http.entity.BasicHttpEntity; import org.apache.http.entity.ContentType; import org.apache.http.entity.StringEntity; import org.apache.http.message.BasicHttpResponse; import org.apache.http.message.BasicStatusLine; import org.apache.http.nio.ContentDecoder; import org.apache.http.nio.IOControl; +import org.apache.http.nio.protocol.HttpAsyncResponseConsumer; import org.apache.http.protocol.HttpContext; +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Modifier; import java.util.concurrent.atomic.AtomicReference; +import static org.hamcrest.CoreMatchers.instanceOf; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; @@ -97,6 +102,26 @@ public class HeapBufferedAsyncResponseConsumerTests extends RestClientTestCase { bufferLimitTest(consumer, bufferLimit); } + public void testCanConfigureHeapBufferLimitFromOutsidePackage() throws ClassNotFoundException, NoSuchMethodException, + IllegalAccessException, InvocationTargetException, InstantiationException { + int bufferLimit = randomIntBetween(1, Integer.MAX_VALUE); + //we use reflection to make sure that the class can be instantiated from the outside, and the constructor is public + Constructor constructor = HttpAsyncResponseConsumerFactory.HeapBufferedResponseConsumerFactory.class.getConstructor(Integer.TYPE); + assertEquals(Modifier.PUBLIC, constructor.getModifiers() & Modifier.PUBLIC); + Object object = constructor.newInstance(bufferLimit); + assertThat(object, instanceOf(HttpAsyncResponseConsumerFactory.HeapBufferedResponseConsumerFactory.class)); + HttpAsyncResponseConsumerFactory.HeapBufferedResponseConsumerFactory consumerFactory = + (HttpAsyncResponseConsumerFactory.HeapBufferedResponseConsumerFactory) object; + HttpAsyncResponseConsumer consumer = consumerFactory.createHttpAsyncResponseConsumer(); + assertThat(consumer, instanceOf(HeapBufferedAsyncResponseConsumer.class)); + HeapBufferedAsyncResponseConsumer bufferedAsyncResponseConsumer = (HeapBufferedAsyncResponseConsumer) consumer; + assertEquals(bufferLimit, bufferedAsyncResponseConsumer.getBufferLimit()); + } + + public void testHttpAsyncResponseConsumerFactoryVisibility() throws ClassNotFoundException { + assertEquals(Modifier.PUBLIC, HttpAsyncResponseConsumerFactory.class.getModifiers() & Modifier.PUBLIC); + } + private static void bufferLimitTest(HeapBufferedAsyncResponseConsumer consumer, int bufferLimit) throws Exception { ProtocolVersion protocolVersion = new ProtocolVersion("HTTP", 1, 1); StatusLine statusLine = new BasicStatusLine(protocolVersion, 200, "OK"); From 9db8a266e6ed6d6557cd9f7aca6b4e5d74e3dad5 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Mon, 10 Apr 2017 12:28:56 +0200 Subject: [PATCH 21/26] Un-deprecate NamedXContentRegistry.Entry constructor that takes a context (#23986) We deprecated this method in the past because we thought it was a temporary thing that could go away over time. We radically trimmed down the usages of a context while parsing when we got rid of the ParseFieldMatcher, but the usages that are left are legit and we will hardly get rid of them. Also, working on aggs parsing we will need a context to carry around the aggregation name that gets parsed through XContentParser#namedObject . --- .../elasticsearch/common/xcontent/NamedXContentRegistry.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/NamedXContentRegistry.java b/core/src/main/java/org/elasticsearch/common/xcontent/NamedXContentRegistry.java index 37ecc579e3e..41593bfe238 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/NamedXContentRegistry.java +++ b/core/src/main/java/org/elasticsearch/common/xcontent/NamedXContentRegistry.java @@ -69,9 +69,8 @@ public class NamedXContentRegistry { } /** * Creates a new entry which can be stored by the registry. - * @deprecated prefer {@link Entry#Entry(Class, ParseField, CheckedFunction)}. Contexts will be removed when possible + * Prefer {@link Entry#Entry(Class, ParseField, CheckedFunction)} unless you need a context to carry around while parsing. */ - @Deprecated public Entry(Class categoryClass, ParseField name, ContextParser parser) { this.categoryClass = Objects.requireNonNull(categoryClass); this.name = Objects.requireNonNull(name); From b283c8b768939ee6e9b9a438d4927709bffa003a Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Mon, 10 Apr 2017 12:30:02 +0200 Subject: [PATCH 22/26] Move aggs CommonFields and TYPED_KEYS_DELIMITER from InternalAggregation to Aggregation (#23987) These will be shared between internal objects and objects exposed through high level REST client, so they should be moved from internal classes. --- .../search/aggregations/Aggregation.java | 26 +++++++++++++++++++ .../aggregations/InternalAggregation.java | 21 --------------- .../bucket/sampler/UnmappedSampler.java | 3 ++- .../elasticsearch/search/suggest/Suggest.java | 6 ++--- 4 files changed, 31 insertions(+), 25 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/Aggregation.java b/core/src/main/java/org/elasticsearch/search/aggregations/Aggregation.java index 41ef321a2c4..991243ce0d4 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/Aggregation.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/Aggregation.java @@ -18,6 +18,8 @@ */ package org.elasticsearch.search.aggregations; +import org.elasticsearch.common.ParseField; + import java.util.Map; /** @@ -25,6 +27,12 @@ import java.util.Map; */ public interface Aggregation { + /** + * Delimiter used when prefixing aggregation names with their type + * using the typed_keys parameter + */ + String TYPED_KEYS_DELIMITER = "#"; + /** * @return The name of this aggregation. */ @@ -34,4 +42,22 @@ public interface Aggregation { * Get the optional byte array metadata that was set on the aggregation */ Map getMetaData(); + + /** + * Common xcontent fields that are shared among addAggregation + */ + final class CommonFields extends ParseField.CommonFields { + public static final ParseField META = new ParseField("meta"); + public static final ParseField BUCKETS = new ParseField("buckets"); + public static final ParseField VALUE = new ParseField("value"); + public static final ParseField VALUES = new ParseField("values"); + public static final ParseField VALUE_AS_STRING = new ParseField("value_as_string"); + public static final ParseField DOC_COUNT = new ParseField("doc_count"); + public static final ParseField KEY = new ParseField("key"); + public static final ParseField KEY_AS_STRING = new ParseField("key_as_string"); + public static final ParseField FROM = new ParseField("from"); + public static final ParseField FROM_AS_STRING = new ParseField("from_as_string"); + public static final ParseField TO = new ParseField("to"); + public static final ParseField TO_AS_STRING = new ParseField("to_as_string"); + } } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/InternalAggregation.java b/core/src/main/java/org/elasticsearch/search/aggregations/InternalAggregation.java index 11382113ab4..0618e5cb29d 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/InternalAggregation.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/InternalAggregation.java @@ -18,7 +18,6 @@ */ package org.elasticsearch.search.aggregations; -import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.NamedWriteable; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -40,9 +39,6 @@ import java.util.Objects; */ public abstract class InternalAggregation implements Aggregation, ToXContent, NamedWriteable { - /** Delimiter used when prefixing aggregation names with their type using the typed_keys parameter **/ - public static final String TYPED_KEYS_DELIMITER = "#"; - public static class ReduceContext { private final BigArrays bigArrays; @@ -242,21 +238,4 @@ public abstract class InternalAggregation implements Aggregation, ToXContent, Na return this == obj; } - /** - * Common xcontent fields that are shared among addAggregation - */ - public static final class CommonFields extends ParseField.CommonFields { - public static final ParseField META = new ParseField("meta"); - public static final ParseField BUCKETS = new ParseField("buckets"); - public static final ParseField VALUE = new ParseField("value"); - public static final ParseField VALUES = new ParseField("values"); - public static final ParseField VALUE_AS_STRING = new ParseField("value_as_string"); - public static final ParseField DOC_COUNT = new ParseField("doc_count"); - public static final ParseField KEY = new ParseField("key"); - public static final ParseField KEY_AS_STRING = new ParseField("key_as_string"); - public static final ParseField FROM = new ParseField("from"); - public static final ParseField FROM_AS_STRING = new ParseField("from_as_string"); - public static final ParseField TO = new ParseField("to"); - public static final ParseField TO_AS_STRING = new ParseField("to_as_string"); - } } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/sampler/UnmappedSampler.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/sampler/UnmappedSampler.java index 5cabb618b8a..3459e110d7e 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/sampler/UnmappedSampler.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/sampler/UnmappedSampler.java @@ -20,6 +20,7 @@ package org.elasticsearch.search.aggregations.bucket.sampler; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.search.aggregations.Aggregation; import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.InternalAggregations; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; @@ -59,7 +60,7 @@ public class UnmappedSampler extends InternalSampler { @Override public XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException { - builder.field(InternalAggregation.CommonFields.DOC_COUNT.getPreferredName(), 0); + builder.field(Aggregation.CommonFields.DOC_COUNT.getPreferredName(), 0); return builder; } diff --git a/core/src/main/java/org/elasticsearch/search/suggest/Suggest.java b/core/src/main/java/org/elasticsearch/search/suggest/Suggest.java index f85ea18109b..f483b5de89f 100644 --- a/core/src/main/java/org/elasticsearch/search/suggest/Suggest.java +++ b/core/src/main/java/org/elasticsearch/search/suggest/Suggest.java @@ -34,7 +34,7 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.rest.action.search.RestSearchAction; -import org.elasticsearch.search.aggregations.InternalAggregation; +import org.elasticsearch.search.aggregations.Aggregation; import org.elasticsearch.search.suggest.Suggest.Suggestion.Entry; import org.elasticsearch.search.suggest.Suggest.Suggestion.Entry.Option; import org.elasticsearch.search.suggest.completion.CompletionSuggestion; @@ -373,7 +373,7 @@ public class Suggest implements Iterable 0) { From 93f159429faf678f52dae2332c73f40b9b57ea3c Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Mon, 10 Apr 2017 12:31:45 +0200 Subject: [PATCH 23/26] Remove getProperty method from Aggregations interface and impl (#23972) The `getProperty` method is an internal method needed to run pipeline aggregations and retrieve info by path from the aggs tree. It is not needed in the `Aggregations` interface, which is returned to users running aggregations from the transport client. Furthermore, the method is currenty unused by pipeline aggs too, as only InternalAggregation#getProperty is used. It can then be removed --- .../search/aggregations/Aggregations.java | 10 ---------- .../aggregations/InternalAggregations.java | 19 ------------------- 2 files changed, 29 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java b/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java index d7a6668ed46..f7fe3e49f0a 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java @@ -45,14 +45,4 @@ public interface Aggregations extends Iterable { * Returns the aggregation that is associated with the specified name. */ A get(String name); - - /** - * Get the value of specified path in the aggregation. - * - * @param path - * the path to the property in the aggregation tree - * @return the value of the property - */ - Object getProperty(String path); - } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/InternalAggregations.java b/core/src/main/java/org/elasticsearch/search/aggregations/InternalAggregations.java index b9fca78ec44..c835c47c148 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/InternalAggregations.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/InternalAggregations.java @@ -24,7 +24,6 @@ import org.elasticsearch.common.io.stream.Streamable; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.search.aggregations.InternalAggregation.ReduceContext; -import org.elasticsearch.search.aggregations.support.AggregationPath; import java.io.IOException; import java.util.ArrayList; @@ -107,24 +106,6 @@ public class InternalAggregations implements Aggregations, ToXContent, Streamabl return (A) asMap().get(name); } - @Override - public Object getProperty(String path) { - AggregationPath aggPath = AggregationPath.parse(path); - return getProperty(aggPath.getPathElementsAsStringList()); - } - - public Object getProperty(List path) { - if (path.isEmpty()) { - return this; - } - String aggName = path.get(0); - InternalAggregation aggregation = get(aggName); - if (aggregation == null) { - throw new IllegalArgumentException("Cannot find an aggregation named [" + aggName + "]"); - } - return aggregation.getProperty(path.subList(1, path.size())); - } - /** * Reduces the given lists of addAggregation. * From af49c46b7676de311dc312471fd8c3e12eee39a8 Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Mon, 10 Apr 2017 12:40:19 +0200 Subject: [PATCH 24/26] Fix BWC tests for field_stats now that the deprecation has been back ported to 5.4 --- .../test/field_stats/10_basics.yaml | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/field_stats/10_basics.yaml b/rest-api-spec/src/main/resources/rest-api-spec/test/field_stats/10_basics.yaml index 907432d2d66..6053d685e2e 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/field_stats/10_basics.yaml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/field_stats/10_basics.yaml @@ -60,8 +60,8 @@ setup: --- "Basic field stats": - skip: - version: " - 5.99.99" - reason: Deprecation was added in 6.0.0 + version: " - 5.3.99" + reason: Deprecation was added in 5.4.0 features: warnings - do: @@ -94,8 +94,8 @@ setup: --- "Geo field stats": - skip: - version: " - 5.99.99" - reason: Deprecation was added in 6.0.0 + version: " - 5.3.99" + reason: Deprecation was added in 5.4.0 features: warnings - do: @@ -127,8 +127,8 @@ setup: --- "Basic field stats with level set to indices": - skip: - version: " - 5.99.99" - reason: Deprecation was added in 6.0.0 + version: " - 5.3.99" + reason: Deprecation was added in 5.4.0 features: warnings - do: @@ -181,8 +181,8 @@ setup: --- "Geo field stats with level set to indices": - skip: - version: " - 5.99.99" - reason: Deprecation was added in 6.0.0 + version: " - 5.3.99" + reason: Deprecation was added in 5.4.0 features: warnings - do: @@ -216,8 +216,8 @@ setup: --- "Geopoint field stats": - skip: - version: " - 5.99.99" - reason: Deprecation was added in 6.0.0 + version: " - 5.3.99" + reason: Deprecation was added in 5.4.0 features: warnings - do: @@ -240,8 +240,8 @@ setup: --- "Field stats with filtering": - skip: - version: " - 5.99.99" - reason: Deprecation was added in 6.0.0 + version: " - 5.3.99" + reason: Deprecation was added in 5.4.0 features: warnings - do: @@ -275,8 +275,8 @@ setup: --- "Field stats both source and fields": - skip: - version: " - 5.99.99" - reason: Deprecation was added in 6.0.0 + version: " - 5.3.99" + reason: Deprecation was added in 5.4.0 features: warnings - do: @@ -289,8 +289,8 @@ setup: --- "Field stats with conflicts": - skip: - version: " - 5.99.99" - reason: Deprecation was added in 6.0.0 + version: " - 5.3.99" + reason: Deprecation was added in 5.4.0 features: warnings - do: From 2c545c064d03e92e17d3a75a3181a4da84be0680 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Mon, 10 Apr 2017 13:35:01 +0200 Subject: [PATCH 25/26] Move getProperty method out of MultiBucketsAggregation.Bucket interface (#23988) The getProperty method is an internal method needed to run pipeline aggregations and retrieve info by path from the aggs tree. It is not needed in the MultiBucketsAggregation.Bucket interface, which is returned to users running aggregations from the transport client. The method is moved to the InternalMultiBucketAggregation class as that's where it belongs. --- .../InternalMultiBucketAggregation.java | 7 +++++-- .../bucket/MultiBucketsAggregation.java | 7 +------ .../bucket/geogrid/GeoHashGrid.java | 3 +-- .../bucket/geogrid/InternalGeoHashGrid.java | 2 +- .../bucket/histogram/Histogram.java | 3 +-- .../histogram/InternalDateHistogram.java | 2 +- .../bucket/histogram/InternalHistogram.java | 2 +- .../bucket/range/InternalBinaryRange.java | 2 +- .../aggregations/pipeline/BucketHelpers.java | 4 ++-- .../BucketMetricsPipelineAggregator.java | 6 ++---- .../BucketScriptPipelineAggregator.java | 18 +++++++--------- .../BucketSelectorPipelineAggregator.java | 8 +++---- .../CumulativeSumPipelineAggregator.java | 11 +++++----- .../DerivativePipelineAggregator.java | 11 +++++----- .../movavg/MovAvgPipelineAggregator.java | 14 ++++++++----- .../SerialDiffPipelineAggregator.java | 21 +++++++++---------- .../bucket/DateHistogramOffsetIT.java | 2 +- .../aggregations/bucket/GeoHashGridIT.java | 2 +- .../aggregations/bucket/HistogramIT.java | 2 +- .../DateHistogramAggregatorTests.java | 16 +++++++------- .../aggregations/metrics/GeoCentroidIT.java | 5 ++--- .../pipeline/DateDerivativeIT.java | 7 +++++-- .../aggregations/pipeline/DerivativeIT.java | 15 +++++++------ .../expression/MoreExpressionTests.java | 2 +- 24 files changed, 86 insertions(+), 86 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregation.java b/core/src/main/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregation.java index a1326aaed11..5317f519819 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregation.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregation.java @@ -62,6 +62,9 @@ public abstract class InternalMultiBucketAggregation getBuckets(); + @Override public Object getProperty(List path) { if (path.isEmpty()) { @@ -69,7 +72,7 @@ public abstract class InternalMultiBucketAggregation buckets = getBuckets(); + List buckets = getBuckets(); Object[] propertyArray = new Object[buckets.size()]; for (int i = 0; i < buckets.size(); i++) { propertyArray[i] = buckets.get(i).getProperty(getName(), path); @@ -79,7 +82,7 @@ public abstract class InternalMultiBucketAggregation path) { if (path.isEmpty()) { return this; diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/MultiBucketsAggregation.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/MultiBucketsAggregation.java index 54b33bb76a0..49c58002227 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/MultiBucketsAggregation.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/MultiBucketsAggregation.java @@ -19,8 +19,6 @@ package org.elasticsearch.search.aggregations.bucket; -import org.elasticsearch.common.io.stream.StreamInput; -import org.elasticsearch.common.io.stream.Streamable; import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.common.util.Comparators; import org.elasticsearch.common.xcontent.ToXContent; @@ -29,7 +27,6 @@ import org.elasticsearch.search.aggregations.Aggregations; import org.elasticsearch.search.aggregations.HasAggregations; import org.elasticsearch.search.aggregations.support.AggregationPath; -import java.io.IOException; import java.util.List; /** @@ -40,7 +37,7 @@ public interface MultiBucketsAggregation extends Aggregation { * A bucket represents a criteria to which all documents that fall in it adhere to. It is also uniquely identified * by a key, and can potentially hold sub-aggregations computed over all documents in it. */ - public interface Bucket extends HasAggregations, ToXContent, Writeable { + interface Bucket extends HasAggregations, ToXContent, Writeable { /** * @return The key associated with the bucket */ @@ -62,8 +59,6 @@ public interface MultiBucketsAggregation extends Aggregation { @Override Aggregations getAggregations(); - Object getProperty(String containingAggName, List path); - class SubAggregationComparator implements java.util.Comparator { private final AggregationPath path; diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGrid.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGrid.java index 6456fba8640..9cce698957d 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGrid.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGrid.java @@ -38,6 +38,5 @@ public interface GeoHashGrid extends MultiBucketsAggregation { * @return The buckets of this aggregation (each bucket representing a geohash grid cell) */ @Override - List getBuckets(); - + List getBuckets(); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGrid.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGrid.java index c1a6ad8be9a..20bccb68305 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGrid.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGrid.java @@ -185,7 +185,7 @@ public class InternalGeoHashGrid extends InternalMultiBucketAggregation getBuckets() { + public List getBuckets() { return unmodifiableList(buckets); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/Histogram.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/Histogram.java index 9453ecef596..3ac87de81ed 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/Histogram.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/Histogram.java @@ -48,8 +48,7 @@ public interface Histogram extends MultiBucketsAggregation { * @return The buckets of this histogram (each bucket representing an interval in the histogram) */ @Override - List getBuckets(); - + List getBuckets(); /** * A strategy defining the order in which the buckets in this histogram are ordered. diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalDateHistogram.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalDateHistogram.java index 9815fdd2144..dc05ab51e8a 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalDateHistogram.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalDateHistogram.java @@ -265,7 +265,7 @@ public final class InternalDateHistogram extends InternalMultiBucketAggregation< } @Override - public List getBuckets() { + public List getBuckets() { return Collections.unmodifiableList(buckets); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalHistogram.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalHistogram.java index 046551fc58a..c6d2aa9eeb9 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalHistogram.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalHistogram.java @@ -255,7 +255,7 @@ public final class InternalHistogram extends InternalMultiBucketAggregation getBuckets() { + public List getBuckets() { return Collections.unmodifiableList(buckets); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRange.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRange.java index ea515863363..640b1cfb467 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRange.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRange.java @@ -225,7 +225,7 @@ public final class InternalBinaryRange } @Override - public List getBuckets() { + public List getBuckets() { return unmodifiableList(buckets); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketHelpers.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketHelpers.java index 9fc1136968e..90665eeb9b0 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketHelpers.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketHelpers.java @@ -147,13 +147,13 @@ public class BucketHelpers { * aggPath */ public static Double resolveBucketValue(MultiBucketsAggregation agg, - InternalMultiBucketAggregation.Bucket bucket, String aggPath, GapPolicy gapPolicy) { + InternalMultiBucketAggregation.InternalBucket bucket, String aggPath, GapPolicy gapPolicy) { List aggPathsList = AggregationPath.parse(aggPath).getPathElementsAsStringList(); return resolveBucketValue(agg, bucket, aggPathsList, gapPolicy); } public static Double resolveBucketValue(MultiBucketsAggregation agg, - InternalMultiBucketAggregation.Bucket bucket, List aggPathAsList, GapPolicy gapPolicy) { + InternalMultiBucketAggregation.InternalBucket bucket, List aggPathAsList, GapPolicy gapPolicy) { try { Object propertyValue = bucket.getProperty(agg.getName(), aggPathAsList); if (propertyValue == null) { diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/BucketMetricsPipelineAggregator.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/BucketMetricsPipelineAggregator.java index 6bdbd5e6e4f..413862d3f1d 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/BucketMetricsPipelineAggregator.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/BucketMetricsPipelineAggregator.java @@ -27,7 +27,6 @@ import org.elasticsearch.search.aggregations.Aggregations; import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.InternalAggregation.ReduceContext; import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation; -import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation.Bucket; import org.elasticsearch.search.aggregations.pipeline.BucketHelpers; import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; @@ -82,9 +81,8 @@ public abstract class BucketMetricsPipelineAggregator extends SiblingPipelineAgg if (aggregation.getName().equals(bucketsPath.get(0))) { bucketsPath = bucketsPath.subList(1, bucketsPath.size()); InternalMultiBucketAggregation multiBucketsAgg = (InternalMultiBucketAggregation) aggregation; - List buckets = multiBucketsAgg.getBuckets(); - for (int i = 0; i < buckets.size(); i++) { - Bucket bucket = buckets.get(i); + List buckets = multiBucketsAgg.getBuckets(); + for (InternalMultiBucketAggregation.InternalBucket bucket : buckets) { Double bucketValue = BucketHelpers.resolveBucketValue(multiBucketsAgg, bucket, bucketsPath, gapPolicy); if (bucketValue != null && !Double.isNaN(bucketValue)) { collectBucketValue(bucket.getKeyAsString(), bucketValue); diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java index 059d2fec037..87df926ebab 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java @@ -31,14 +31,12 @@ import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.InternalAggregation.ReduceContext; import org.elasticsearch.search.aggregations.InternalAggregations; import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation; -import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation.Bucket; import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; import org.elasticsearch.search.aggregations.pipeline.InternalSimpleValue; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import java.io.IOException; import java.util.ArrayList; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -89,12 +87,13 @@ public class BucketScriptPipelineAggregator extends PipelineAggregator { @Override public InternalAggregation reduce(InternalAggregation aggregation, ReduceContext reduceContext) { - InternalMultiBucketAggregation originalAgg = (InternalMultiBucketAggregation) aggregation; - List buckets = originalAgg.getBuckets(); + InternalMultiBucketAggregation originalAgg = + (InternalMultiBucketAggregation) aggregation; + List buckets = originalAgg.getBuckets(); CompiledScript compiledScript = reduceContext.scriptService().compile(script, ScriptContext.Standard.AGGS); - List newBuckets = new ArrayList<>(); - for (Bucket bucket : buckets) { + List newBuckets = new ArrayList<>(); + for (InternalMultiBucketAggregation.InternalBucket bucket : buckets) { Map vars = new HashMap<>(); if (script.getParams() != null) { vars.putAll(script.getParams()); @@ -122,13 +121,12 @@ public class BucketScriptPipelineAggregator extends PipelineAggregator { throw new AggregationExecutionException("series_arithmetic script for reducer [" + name() + "] must return a Number"); } - final List aggs = StreamSupport.stream(bucket.getAggregations().spliterator(), false).map((p) -> { - return (InternalAggregation) p; - }).collect(Collectors.toList()); + final List aggs = StreamSupport.stream(bucket.getAggregations().spliterator(), false).map( + (p) -> (InternalAggregation) p).collect(Collectors.toList()); aggs.add(new InternalSimpleValue(name(), ((Number) returned).doubleValue(), formatter, new ArrayList<>(), metaData())); InternalMultiBucketAggregation.InternalBucket newBucket = originalAgg.createBucket(new InternalAggregations(aggs), - (InternalMultiBucketAggregation.InternalBucket) bucket); + bucket); newBuckets.add(newBucket); } } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketselector/BucketSelectorPipelineAggregator.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketselector/BucketSelectorPipelineAggregator.java index f45ebd1c5a3..62eed8d4e0a 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketselector/BucketSelectorPipelineAggregator.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketselector/BucketSelectorPipelineAggregator.java @@ -29,13 +29,11 @@ import org.elasticsearch.script.ScriptContext; import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.InternalAggregation.ReduceContext; import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation; -import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation.Bucket; import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import java.io.IOException; import java.util.ArrayList; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -84,11 +82,11 @@ public class BucketSelectorPipelineAggregator extends PipelineAggregator { public InternalAggregation reduce(InternalAggregation aggregation, ReduceContext reduceContext) { InternalMultiBucketAggregation originalAgg = (InternalMultiBucketAggregation) aggregation; - List buckets = originalAgg.getBuckets(); + List buckets = originalAgg.getBuckets(); CompiledScript compiledScript = reduceContext.scriptService().compile(script, ScriptContext.Standard.AGGS); - List newBuckets = new ArrayList<>(); - for (Bucket bucket : buckets) { + List newBuckets = new ArrayList<>(); + for (InternalMultiBucketAggregation.InternalBucket bucket : buckets) { Map vars = new HashMap<>(); if (script.getParams() != null) { vars.putAll(script.getParams()); diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/cumulativesum/CumulativeSumPipelineAggregator.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/cumulativesum/CumulativeSumPipelineAggregator.java index 98c6f7b2fa2..e79ba1047e2 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/cumulativesum/CumulativeSumPipelineAggregator.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/cumulativesum/CumulativeSumPipelineAggregator.java @@ -25,7 +25,7 @@ import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.InternalAggregation.ReduceContext; import org.elasticsearch.search.aggregations.InternalAggregations; -import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation; +import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation; import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation.Bucket; import org.elasticsearch.search.aggregations.bucket.histogram.HistogramFactory; import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; @@ -70,13 +70,14 @@ public class CumulativeSumPipelineAggregator extends PipelineAggregator { @Override public InternalAggregation reduce(InternalAggregation aggregation, ReduceContext reduceContext) { - MultiBucketsAggregation histo = (MultiBucketsAggregation) aggregation; - List buckets = histo.getBuckets(); + InternalMultiBucketAggregation + histo = (InternalMultiBucketAggregation) aggregation; + List buckets = histo.getBuckets(); HistogramFactory factory = (HistogramFactory) histo; - List newBuckets = new ArrayList<>(); double sum = 0; - for (Bucket bucket : buckets) { + for (InternalMultiBucketAggregation.InternalBucket bucket : buckets) { Double thisBucketValue = resolveBucketValue(histo, bucket, bucketsPaths()[0], GapPolicy.INSERT_ZEROS); sum += thisBucketValue; List aggs = StreamSupport.stream(bucket.getAggregations().spliterator(), false).map((p) -> { diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/derivative/DerivativePipelineAggregator.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/derivative/DerivativePipelineAggregator.java index 480f04f545a..3fe60f23cf3 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/derivative/DerivativePipelineAggregator.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/derivative/DerivativePipelineAggregator.java @@ -25,7 +25,7 @@ import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.InternalAggregation.ReduceContext; import org.elasticsearch.search.aggregations.InternalAggregations; -import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation; +import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation; import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation.Bucket; import org.elasticsearch.search.aggregations.bucket.histogram.HistogramFactory; import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; @@ -77,14 +77,16 @@ public class DerivativePipelineAggregator extends PipelineAggregator { @Override public InternalAggregation reduce(InternalAggregation aggregation, ReduceContext reduceContext) { - MultiBucketsAggregation histo = (MultiBucketsAggregation) aggregation; - List buckets = histo.getBuckets(); + InternalMultiBucketAggregation + histo = (InternalMultiBucketAggregation) aggregation; + List buckets = histo.getBuckets(); HistogramFactory factory = (HistogramFactory) histo; List newBuckets = new ArrayList<>(); Number lastBucketKey = null; Double lastBucketValue = null; - for (Bucket bucket : buckets) { + for (InternalMultiBucketAggregation.InternalBucket bucket : buckets) { Number thisBucketKey = factory.getKey(bucket); Double thisBucketValue = resolveBucketValue(histo, bucket, bucketsPaths()[0], gapPolicy); if (lastBucketValue != null && thisBucketValue != null) { @@ -107,5 +109,4 @@ public class DerivativePipelineAggregator extends PipelineAggregator { } return factory.createAggregation(newBuckets); } - } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/MovAvgPipelineAggregator.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/MovAvgPipelineAggregator.java index 87aa5bfda63..49422995c95 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/MovAvgPipelineAggregator.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/MovAvgPipelineAggregator.java @@ -26,6 +26,7 @@ import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.InternalAggregation.ReduceContext; import org.elasticsearch.search.aggregations.InternalAggregations; +import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation; import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation; import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation.Bucket; import org.elasticsearch.search.aggregations.bucket.histogram.HistogramFactory; @@ -93,8 +94,10 @@ public class MovAvgPipelineAggregator extends PipelineAggregator { @Override public InternalAggregation reduce(InternalAggregation aggregation, ReduceContext reduceContext) { - MultiBucketsAggregation histo = (MultiBucketsAggregation) aggregation; - List buckets = histo.getBuckets(); + InternalMultiBucketAggregation + histo = (InternalMultiBucketAggregation) aggregation; + List buckets = histo.getBuckets(); HistogramFactory factory = (HistogramFactory) histo; List newBuckets = new ArrayList<>(); @@ -110,7 +113,7 @@ public class MovAvgPipelineAggregator extends PipelineAggregator { model = minimize(buckets, histo, model); } - for (Bucket bucket : buckets) { + for (InternalMultiBucketAggregation.InternalBucket bucket : buckets) { Double thisBucketValue = resolveBucketValue(histo, bucket, bucketsPaths()[0], gapPolicy); // Default is to reuse existing bucket. Simplifies the rest of the logic, @@ -180,13 +183,14 @@ public class MovAvgPipelineAggregator extends PipelineAggregator { return factory.createAggregation(newBuckets); } - private MovAvgModel minimize(List buckets, MultiBucketsAggregation histo, MovAvgModel model) { + private MovAvgModel minimize(List buckets, + MultiBucketsAggregation histo, MovAvgModel model) { int counter = 0; EvictingQueue values = new EvictingQueue<>(this.window); double[] test = new double[window]; - ListIterator iter = buckets.listIterator(buckets.size()); + ListIterator iter = buckets.listIterator(buckets.size()); // We have to walk the iterator backwards because we don't know if/how many buckets are empty. while (iter.hasPrevious() && counter < window) { diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/serialdiff/SerialDiffPipelineAggregator.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/serialdiff/SerialDiffPipelineAggregator.java index 3216d5527dc..d438104be7f 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/serialdiff/SerialDiffPipelineAggregator.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/serialdiff/SerialDiffPipelineAggregator.java @@ -26,10 +26,10 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.InternalAggregation.ReduceContext; -import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation; +import org.elasticsearch.search.aggregations.InternalAggregations; +import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation; import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation.Bucket; import org.elasticsearch.search.aggregations.bucket.histogram.HistogramFactory; -import org.elasticsearch.search.aggregations.InternalAggregations; import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; import org.elasticsearch.search.aggregations.pipeline.InternalSimpleValue; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; @@ -80,15 +80,17 @@ public class SerialDiffPipelineAggregator extends PipelineAggregator { @Override public InternalAggregation reduce(InternalAggregation aggregation, ReduceContext reduceContext) { - MultiBucketsAggregation histo = (MultiBucketsAggregation) aggregation; - List buckets = histo.getBuckets(); + InternalMultiBucketAggregation + histo = (InternalMultiBucketAggregation) aggregation; + List buckets = histo.getBuckets(); HistogramFactory factory = (HistogramFactory) histo; List newBuckets = new ArrayList<>(); EvictingQueue lagWindow = new EvictingQueue<>(lag); int counter = 0; - for (Bucket bucket : buckets) { + for (InternalMultiBucketAggregation.InternalBucket bucket : buckets) { Double thisBucketValue = resolveBucketValue(histo, bucket, bucketsPaths()[0], gapPolicy); Bucket newBucket = bucket; @@ -111,17 +113,14 @@ public class SerialDiffPipelineAggregator extends PipelineAggregator { if (!Double.isNaN(thisBucketValue) && !Double.isNaN(lagValue)) { double diff = thisBucketValue - lagValue; - List aggs = StreamSupport.stream(bucket.getAggregations().spliterator(), false).map((p) -> { - return (InternalAggregation) p; - }).collect(Collectors.toList()); - aggs.add(new InternalSimpleValue(name(), diff, formatter, new ArrayList(), metaData())); + List aggs = StreamSupport.stream(bucket.getAggregations().spliterator(), false).map( + (p) -> (InternalAggregation) p).collect(Collectors.toList()); + aggs.add(new InternalSimpleValue(name(), diff, formatter, new ArrayList<>(), metaData())); newBucket = factory.createBucket(factory.getKey(bucket), bucket.getDocCount(), new InternalAggregations(aggs)); } - newBuckets.add(newBucket); lagWindow.add(thisBucketValue); - } return factory.createAggregation(newBuckets); } diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateHistogramOffsetIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateHistogramOffsetIT.java index dcc80f69903..5e56f753274 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateHistogramOffsetIT.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateHistogramOffsetIT.java @@ -88,7 +88,7 @@ public class DateHistogramOffsetIT extends ESIntegTestCase { assertThat(response.getHits().getTotalHits(), equalTo(5L)); Histogram histo = response.getAggregations().get("date_histo"); - List buckets = histo.getBuckets(); + List buckets = histo.getBuckets(); assertThat(buckets.size(), equalTo(2)); checkBucketFor(buckets.get(0), new DateTime(2014, 3, 10, 2, 0, DateTimeZone.UTC), 2L); diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridIT.java index 4cf6cfac9cb..7f158e0732f 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridIT.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridIT.java @@ -157,7 +157,7 @@ public class GeoHashGridIT extends ESIntegTestCase { assertSearchResponse(response); GeoHashGrid geoGrid = response.getAggregations().get("geohashgrid"); - List buckets = geoGrid.getBuckets(); + List buckets = geoGrid.getBuckets(); Object[] propertiesKeys = (Object[]) ((InternalAggregation)geoGrid).getProperty("_key"); Object[] propertiesDocCounts = (Object[]) ((InternalAggregation)geoGrid).getProperty("_count"); for (int i = 0; i < buckets.size(); i++) { diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramIT.java index 444e1568cdb..683e7924419 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramIT.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramIT.java @@ -990,7 +990,7 @@ public class HistogramIT extends ESIntegTestCase { assertSearchResponse(r); Histogram histogram = r.getAggregations().get("histo"); - List buckets = histogram.getBuckets(); + List buckets = histogram.getBuckets(); assertEquals(2, buckets.size()); assertEquals(-0.65, (double) buckets.get(0).getKey(), 0.01d); assertEquals(1, buckets.get(0).getDocCount()); diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregatorTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregatorTests.java index 1ad8f950b9a..9d3374630a9 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregatorTests.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregatorTests.java @@ -105,7 +105,7 @@ public class DateHistogramAggregatorTests extends AggregatorTestCase { testBothCases(LongPoint.newRangeQuery(INSTANT_FIELD, asLong("2015-01-01"), asLong("2017-12-31")), dataset, aggregation -> aggregation.dateHistogramInterval(DateHistogramInterval.YEAR).field(DATE_FIELD), histogram -> { - List buckets = histogram.getBuckets(); + List buckets = histogram.getBuckets(); assertEquals(3, buckets.size()); Histogram.Bucket bucket = buckets.get(0); @@ -128,7 +128,7 @@ public class DateHistogramAggregatorTests extends AggregatorTestCase { Arrays.asList("2017-01-01", "2017-02-02", "2017-02-03", "2017-03-04", "2017-03-05", "2017-03-06"), aggregation -> aggregation.dateHistogramInterval(DateHistogramInterval.MONTH).field(DATE_FIELD), histogram -> { - List buckets = histogram.getBuckets(); + List buckets = histogram.getBuckets(); assertEquals(3, buckets.size()); Histogram.Bucket bucket = buckets.get(0); @@ -159,7 +159,7 @@ public class DateHistogramAggregatorTests extends AggregatorTestCase { ), aggregation -> aggregation.dateHistogramInterval(DateHistogramInterval.DAY).field(DATE_FIELD).minDocCount(1L), histogram -> { - List buckets = histogram.getBuckets(); + List buckets = histogram.getBuckets(); assertEquals(4, buckets.size()); Histogram.Bucket bucket = buckets.get(0); @@ -197,7 +197,7 @@ public class DateHistogramAggregatorTests extends AggregatorTestCase { ), aggregation -> aggregation.dateHistogramInterval(DateHistogramInterval.HOUR).field(DATE_FIELD).minDocCount(1L), histogram -> { - List buckets = histogram.getBuckets(); + List buckets = histogram.getBuckets(); assertEquals(6, buckets.size()); Histogram.Bucket bucket = buckets.get(0); @@ -238,7 +238,7 @@ public class DateHistogramAggregatorTests extends AggregatorTestCase { ), aggregation -> aggregation.dateHistogramInterval(DateHistogramInterval.MINUTE).field(DATE_FIELD).minDocCount(1L), histogram -> { - List buckets = histogram.getBuckets(); + List buckets = histogram.getBuckets(); assertEquals(3, buckets.size()); Histogram.Bucket bucket = buckets.get(0); @@ -268,7 +268,7 @@ public class DateHistogramAggregatorTests extends AggregatorTestCase { ), aggregation -> aggregation.dateHistogramInterval(DateHistogramInterval.SECOND).field(DATE_FIELD).minDocCount(1L), histogram -> { - List buckets = histogram.getBuckets(); + List buckets = histogram.getBuckets(); assertEquals(3, buckets.size()); Histogram.Bucket bucket = buckets.get(0); @@ -300,7 +300,7 @@ public class DateHistogramAggregatorTests extends AggregatorTestCase { testSearchAndReduceCase(query, timestamps, aggregation -> aggregation.dateHistogramInterval(DateHistogramInterval.seconds(5)).field(DATE_FIELD).minDocCount(0L), histogram -> { - List buckets = histogram.getBuckets(); + List buckets = histogram.getBuckets(); assertEquals(4, buckets.size()); Histogram.Bucket bucket = buckets.get(0); @@ -325,7 +325,7 @@ public class DateHistogramAggregatorTests extends AggregatorTestCase { testSearchAndReduceCase(query, timestamps, aggregation -> aggregation.dateHistogramInterval(DateHistogramInterval.seconds(5)).field(DATE_FIELD).minDocCount(3L), histogram -> { - List buckets = histogram.getBuckets(); + List buckets = histogram.getBuckets(); assertEquals(1, buckets.size()); Histogram.Bucket bucket = buckets.get(0); diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidIT.java index f4ac33b770b..6f2c979936f 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidIT.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidIT.java @@ -157,9 +157,8 @@ public class GeoCentroidIT extends AbstractGeoTestCase { GeoHashGrid grid = response.getAggregations().get("geoGrid"); assertThat(grid, notNullValue()); assertThat(grid.getName(), equalTo("geoGrid")); - List buckets = grid.getBuckets(); - for (int i=0; i < buckets.size(); ++i) { - GeoHashGrid.Bucket cell = buckets.get(i); + List buckets = grid.getBuckets(); + for (GeoHashGrid.Bucket cell : buckets) { String geohash = cell.getKeyAsString(); GeoPoint expectedCentroid = expectedCentroidsForGeoHash.get(geohash); GeoCentroid centroidAgg = cell.getAggregations().get(aggName); diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/DateDerivativeIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/DateDerivativeIT.java index 25e23ed6568..34bf83b122b 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/DateDerivativeIT.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/DateDerivativeIT.java @@ -23,6 +23,7 @@ import org.elasticsearch.action.index.IndexRequestBuilder; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.index.mapper.DateFieldMapper; import org.elasticsearch.search.aggregations.InternalAggregation; +import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation; import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval; import org.elasticsearch.search.aggregations.bucket.histogram.Histogram; import org.elasticsearch.search.aggregations.bucket.histogram.Histogram.Bucket; @@ -381,7 +382,8 @@ public class DateDerivativeIT extends ESIntegTestCase { deriv = bucket.getAggregations().get("deriv"); assertThat(deriv, notNullValue()); assertThat(deriv.value(), equalTo(4.0)); - assertThat((double) bucket.getProperty("histo", AggregationPath.parse("deriv.value").getPathElementsAsStringList()), equalTo(4.0)); + assertThat(((InternalMultiBucketAggregation.InternalBucket)bucket).getProperty( + "histo", AggregationPath.parse("deriv.value").getPathElementsAsStringList()), equalTo(4.0)); assertThat((DateTime) propertiesKeys[1], equalTo(key)); assertThat((long) propertiesDocCounts[1], equalTo(2L)); assertThat((double) propertiesCounts[1], equalTo(5.0)); @@ -398,7 +400,8 @@ public class DateDerivativeIT extends ESIntegTestCase { deriv = bucket.getAggregations().get("deriv"); assertThat(deriv, notNullValue()); assertThat(deriv.value(), equalTo(10.0)); - assertThat((double) bucket.getProperty("histo", AggregationPath.parse("deriv.value").getPathElementsAsStringList()), equalTo(10.0)); + assertThat(((InternalMultiBucketAggregation.InternalBucket)bucket).getProperty( + "histo", AggregationPath.parse("deriv.value").getPathElementsAsStringList()), equalTo(10.0)); assertThat((DateTime) propertiesKeys[2], equalTo(key)); assertThat((long) propertiesDocCounts[2], equalTo(3L)); assertThat((double) propertiesCounts[2], equalTo(15.0)); diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/DerivativeIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/DerivativeIT.java index 8d7d98c4e80..5561b61f492 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/DerivativeIT.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/DerivativeIT.java @@ -26,6 +26,7 @@ import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.search.aggregations.InternalAggregation; +import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation; import org.elasticsearch.search.aggregations.bucket.histogram.Histogram; import org.elasticsearch.search.aggregations.bucket.histogram.Histogram.Bucket; import org.elasticsearch.search.aggregations.metrics.stats.Stats; @@ -279,7 +280,8 @@ public class DerivativeIT extends ESIntegTestCase { assertThat(sumDeriv, notNullValue()); long sumDerivValue = expectedSum - expectedSumPreviousBucket; assertThat(sumDeriv.value(), equalTo((double) sumDerivValue)); - assertThat((double) bucket.getProperty("histo", AggregationPath.parse("deriv.value").getPathElementsAsStringList()), + assertThat(((InternalMultiBucketAggregation.InternalBucket)bucket).getProperty("histo", + AggregationPath.parse("deriv.value").getPathElementsAsStringList()), equalTo((double) sumDerivValue)); } else { assertThat(sumDeriv, nullValue()); @@ -324,7 +326,8 @@ public class DerivativeIT extends ESIntegTestCase { assertThat(sumDeriv, notNullValue()); long sumDerivValue = expectedSum - expectedSumPreviousBucket; assertThat(sumDeriv.value(), equalTo((double) sumDerivValue)); - assertThat((double) bucket.getProperty("histo", AggregationPath.parse("deriv.value").getPathElementsAsStringList()), + assertThat(((InternalMultiBucketAggregation.InternalBucket)bucket).getProperty("histo", + AggregationPath.parse("deriv.value").getPathElementsAsStringList()), equalTo((double) sumDerivValue)); } else { assertThat(sumDeriv, nullValue()); @@ -451,7 +454,7 @@ public class DerivativeIT extends ESIntegTestCase { Histogram deriv = searchResponse.getAggregations().get("histo"); assertThat(deriv, Matchers.notNullValue()); assertThat(deriv.getName(), equalTo("histo")); - List buckets = deriv.getBuckets(); + List buckets = deriv.getBuckets(); assertThat(buckets.size(), equalTo(valueCounts_empty.length)); for (int i = 0; i < valueCounts_empty.length; i++) { @@ -480,7 +483,7 @@ public class DerivativeIT extends ESIntegTestCase { Histogram deriv = searchResponse.getAggregations().get("histo"); assertThat(deriv, Matchers.notNullValue()); assertThat(deriv.getName(), equalTo("histo")); - List buckets = deriv.getBuckets(); + List buckets = deriv.getBuckets(); assertThat(buckets.size(), equalTo(valueCounts_empty.length)); double lastSumValue = Double.NaN; @@ -522,7 +525,7 @@ public class DerivativeIT extends ESIntegTestCase { Histogram deriv = searchResponse.getAggregations().get("histo"); assertThat(deriv, Matchers.notNullValue()); assertThat(deriv.getName(), equalTo("histo")); - List buckets = deriv.getBuckets(); + List buckets = deriv.getBuckets(); assertThat(buckets.size(), equalTo(valueCounts_empty.length)); double lastSumValue = Double.NaN; @@ -561,7 +564,7 @@ public class DerivativeIT extends ESIntegTestCase { Histogram deriv = searchResponse.getAggregations().get("histo"); assertThat(deriv, Matchers.notNullValue()); assertThat(deriv.getName(), equalTo("histo")); - List buckets = deriv.getBuckets(); + List buckets = deriv.getBuckets(); assertThat(buckets.size(), equalTo(numBuckets_empty_rnd)); double lastSumValue = Double.NaN; diff --git a/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/MoreExpressionTests.java b/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/MoreExpressionTests.java index 1b6789682e1..b0cbc70a676 100644 --- a/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/MoreExpressionTests.java +++ b/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/MoreExpressionTests.java @@ -616,7 +616,7 @@ public class MoreExpressionTests extends ESIntegTestCase { Histogram histogram = response.getAggregations().get("histogram"); assertThat(histogram, notNullValue()); assertThat(histogram.getName(), equalTo("histogram")); - List buckets = histogram.getBuckets(); + List buckets = histogram.getBuckets(); for (int bucketCount = 0; bucketCount < buckets.size(); ++bucketCount) { Histogram.Bucket bucket = buckets.get(bucketCount); From 37aadb2adfa8d385d8a0684e3eba6beb89915ca6 Mon Sep 17 00:00:00 2001 From: David Roberts Date: Mon, 10 Apr 2017 12:37:42 +0100 Subject: [PATCH 26/26] Add the ability to include extra notices in a plugin's NOTICES file (#23898) Adds the option for a plugin to specify extra directories containing notices and licenses files to be incorporated into the overall notices file that is generated for the plugin. This can be useful, for example, where the plugin has a non-Java dependency that itself incorporates many 3rd party components. --- .../elasticsearch/gradle/NoticeTask.groovy | 43 +++++++++++++------ .../gradle/plugin/PluginBuildPlugin.groovy | 1 - distribution/build.gradle | 9 ++-- 3 files changed, 36 insertions(+), 17 deletions(-) diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/NoticeTask.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/NoticeTask.groovy index 9cbdbb41a33..928298db7bf 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/NoticeTask.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/NoticeTask.groovy @@ -37,16 +37,21 @@ public class NoticeTask extends DefaultTask { @OutputFile File outputFile = new File(project.buildDir, "notices/${name}/NOTICE.txt") - /** Configurations to inspect dependencies*/ - private List dependencies = new ArrayList<>() + /** Directories to include notices from */ + private List licensesDirs = new ArrayList<>() public NoticeTask() { description = 'Create a notice file from dependencies' + // Default licenses directory is ${projectDir}/licenses (if it exists) + File licensesDir = new File(project.projectDir, 'licenses') + if (licensesDir.exists()) { + licensesDirs.add(licensesDir) + } } - /** Add notices from licenses found in the given project. */ - public void dependencies(Project project) { - dependencies.add(project) + /** Add notices from the specified directory. */ + public void licensesDir(File licensesDir) { + licensesDirs.add(licensesDir) } @TaskAction @@ -54,17 +59,29 @@ public class NoticeTask extends DefaultTask { StringBuilder output = new StringBuilder() output.append(inputFile.getText('UTF-8')) output.append('\n\n') - Set seen = new HashSet<>() - for (Project dep : dependencies) { - File licensesDir = new File(dep.projectDir, 'licenses') - if (licensesDir.exists() == false) continue - licensesDir.eachFileMatch({ it ==~ /.*-NOTICE\.txt/ && seen.contains(it) == false}) { File file -> + // This is a map rather than a set so that the sort order is the 3rd + // party component names, unaffected by the full path to the various files + Map seen = new TreeMap<>() + for (File licensesDir : licensesDirs) { + licensesDir.eachFileMatch({ it ==~ /.*-NOTICE\.txt/ }) { File file -> String name = file.name.substring(0, file.name.length() - '-NOTICE.txt'.length()) - appendFile(file, name, 'NOTICE', output) - appendFile(new File(file.parentFile, "${name}-LICENSE.txt"), name, 'LICENSE', output) - seen.add(file.name) + if (seen.containsKey(name)) { + File prevFile = seen.get(name) + if (prevFile.text != file.text) { + throw new RuntimeException("Two different notices exist for dependency '" + + name + "': " + prevFile + " and " + file) + } + } else { + seen.put(name, file) + } } } + for (Map.Entry entry : seen.entrySet()) { + String name = entry.getKey() + File file = entry.getValue() + appendFile(file, name, 'NOTICE', output) + appendFile(new File(file.parentFile, "${name}-LICENSE.txt"), name, 'LICENSE', output) + } outputFile.setText(output.toString(), 'UTF-8') } diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy index e83ffa1f111..4b3867a985d 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy @@ -260,7 +260,6 @@ public class PluginBuildPlugin extends BuildPlugin { File noticeFile = project.pluginProperties.extension.noticeFile if (noticeFile != null) { NoticeTask generateNotice = project.tasks.create('generateNotice', NoticeTask.class) - generateNotice.dependencies(project) generateNotice.inputFile = noticeFile project.bundlePlugin.from(generateNotice) } diff --git a/distribution/build.gradle b/distribution/build.gradle index 2a8094eb800..8946d302473 100644 --- a/distribution/build.gradle +++ b/distribution/build.gradle @@ -49,12 +49,12 @@ Collection distributions = project.subprojects.findAll { // integ test zip only uses core, so a different notice file is needed there task buildCoreNotice(type: NoticeTask) { - dependencies project(':core') + licensesDir new File(project(':core').projectDir, 'licenses') } // other distributions include notices from modules as well, which are added below later task buildFullNotice(type: NoticeTask) { - dependencies project(':core') + licensesDir new File(project(':core').projectDir, 'licenses') } /***************************************************************************** @@ -73,7 +73,10 @@ ext.restTestExpansions = [ // loop over modules to also setup cross task dependencies and increment our modules counter project.rootProject.subprojects.findAll { it.path.startsWith(':modules:') }.each { Project module -> buildFullNotice { - dependencies module + def defaultLicensesDir = new File(module.projectDir, 'licenses') + if (defaultLicensesDir.exists()) { + licensesDir defaultLicensesDir + } } buildModules { dependsOn({ project(module.path).bundlePlugin })