HLRest: Allow caller to set per request options #30490

nik9000 · 2018-05-09T18:13:47Z

Updated description based on what I actually committed:

This modifies the high level rest client to allow calling code to
customize per request options for the bulk API. You do the actual
customization by passing a RequestOptions object to the API call
which is set on the Request that is generated by the high level
client. It also makes the RequestOptions a thing in the low level
rest client. For now that just means you use it to customize the
headers and the httpAsyncResponseConsumerFactory and we'll add
node selectors and per request timeouts in a follow up.

I only implemented this on the bulk API because it is the first one
in the list alphabetically and I wanted to keep the change small
enough to review. I'll convert the remaining APIs in a followup.

Original description:

This modifies the high level rest client to allow calling code to
customize per request options for the bulk API. You do the actual
customization by passing a Consumer to the API call which is called
with a "safe" wrapper around the Request that is generated by the high
level client. This "safe" wrapper allows you to change what is safe to
change on a per request basis. For now that just means the headers and
the httpAsyncResponseConsumerFactory but in the future that would mean
changing the node selector or adding per request timeouts.

I picked bulk because iti was the first entry in the list and
demonstrates how we will modify all other requests. I did only a single
request to keep the change small enough to review. I plan to follow up
this change with one converting the other APIs once we've decided that
this is the right way to go.

This modifies the high level rest client to allow calling code to customize per request options for the bulk API. You do the actual customization by passing a `Consumer` to the API call which is called with a "safe" wrapper around the `Request` that is generated by the high level client. This "safe" wrapper allows you to change what is safe to change on a per request basis. For now that just means the headers and the `httpAsyncResponseConsumerFactory` but in the future that would mean changing the node selector or adding per request timeouts. I picked bulk because iti was the first entry in the list and demonstrates how we will modify all other requests. I did only a single request to keep the change small enough to review. I plan to follow up this change with one converting the other APIs once we've decided that this is the right way to go.

elasticmachine · 2018-05-09T18:13:48Z

Pinging @elastic/es-core-infra

javanna · 2018-05-11T12:43:49Z

heya @nik9000 I am not a big fan of the consumer, but I think the bigger problem is the additional two methods per API required. How about grouping the mutable request parts into a new Request.Options class, that can be set to any Request in the low-level client. The high-level client then doesn't need a consumer, but rather something like bulk(BulkRequest request, Request.Options options). In this case we could also have a constant for empty options and require them for all methods (if you don't want to set anything, you provide the constant), removing the need for the additional methods, which would only be needed for backwards compatibility in 6.x.
The consumer saves us a bit of work, as it allows to set stuff safely directly to the request, but knowing that options cannot be ever set unless they are provided when using the high-level client, we just have to take them and set them back to the low-level request? Am I missing something in this proposal?

nik9000 · 2018-05-11T14:08:26Z

I'm happy to drop the second methods! I think they create a ton of noise
and don't save much pain. We'd just keep the Header... versions for
backwards compatibility and drop them from master. I'll do that!

I don't really think that something like Request.Options would be more
user friendly. I played a bit with both options locally and they seem about
the same as far as readability goes. The Consumer is a line shorter here
and there but I don't think that matters. I suspect lots of folks are going
to pre-make the customizers that they need and reuse them over and over
again anyway.

Actually that makes me think of a thing: Consumer<SafeRequest> feels a
little better if you are just going to make it one time and save it because
the usual way to declare these things makes them immutable.
Request.Options would be mutable which seems to me that it'd lead to bugs
when folks try to reuse them. Someone is going to mutate a shared options
instance of cause all sorts of sneaky things to happen.

The examples I was playing with:

BulkRequest request = new BulkRequest();
request.add(new IndexRequest("test", "test").source("test", "test"));

client.bulk(request, r -> r.setNodeSelector(NodeSelector.NOT_MASTER_ONLY));
// or
client.bulk(request, r -> {
    r.setNodeSelector(NodeSelector.NOT_MASTER_ONLY);
    r.setHeaders(new BasicHeader("stuff", "fluff"));
});
// or
client.bulk(request, HighLevelClient.NO_CUSTOMIZATION);

BulkRequest request = new BulkRequest();
request.add(new IndexRequest("test", "test").source("test", "test"));

Request.Options options = new Request.Options();
options.setNodeSelector(NodeSelector.NOT_MASTER_ONLY);
client.bulk(request, options);
// or
Request.Options options = new Request.Options();
options.setNodeSelector(NodeSelector.NOT_MASTER_ONLY);
options.setHeaders(new BasicHEader("stuff", "fluff"));
client.bulk(request, options);
// or
client.bulk(request, Request.Options.EMPTY);

javanna · 2018-05-11T14:44:19Z

I still think that an Options object in the low-level client that holds timeout, headers etc. different from the request itself would be a good notion and unifying the concepts between low-level and high-level would help users as well. In the high-level client, users cannot customize the request itself as it is generated internally, but they can tweak the options only.

If we are afraid of reusing these options and their mutability, we could make them immutable and/or we later expose a way to set default options for all requests in the high-level client, then merge them with the request ones ourselves.

I am not liking having to add a consumer argument to each API method. I find that an internal concept and confusing to users. I don't think replacing the consumer with options is the perfect solution, but a bit better from a user perspective, and clearer.

Given that we have quite opposite opinions, I think it would be good to get opinions from others. @cbuescher @tlrx @spinscale @dakrone do you have thoughts on this? Please feel free to propose alternatives as well!

tlrx · 2018-05-14T10:21:07Z

I think we could give @javanna 's suggestion a try. Using request options looks more familiar to me for a client library, and I'd be happy if default options to use for all requests could be set at the low level rest client and custom options for a given request passed when executing the request. I'm not a big fan of builders but I think in this case it could help to draw the line between the mutable/immutable options with something like:

Options.Builder  = defaultOptions.toBuilder();
bulkOptions.setNodeSelector(NodeSelector.NOT_MASTER_ONLY);
client.bulk(request, bulkOptions); // use all default options/headers but different node selector

dakrone · 2018-05-15T16:35:09Z

I think my preference would be an immutable Options class (or a Builder, for iterative option building) that can be reused:

Options.Builder opts = Options.builder(defaults);
opts.setNodeSelector(...);
opts.addHeader("X-Header", "potatoes");

client.bulk(request, opts); // automatically call .build() on builder

client.search(request, opts.build()); // reusable

I don't think having it be a method buys us anything here. So I'd prefer the
basic options object.

cbuescher · 2018-05-16T13:35:52Z

What @dakrone said. Making the Options immbutable would be great, and I wouldn't mind having a builder in that case. Its a common thing for these kind of use cases I think.

nik9000 · 2018-05-21T19:14:14Z

@dakrone, @cbuescher, @javanna, and @tlrx, could you have a look at afb800f to see if it looks like a start to the thing you want? I think it is a much more complex way of handling the problem then the way that I proposed at first but if you all like it better I'm happy to do it.

javanna · 2018-05-22T19:06:39Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

 * @return the response returned by Elasticsearch
 * @throws IOException in case of a problem or the connection was aborted
 * @throws ClientProtocolException in case of an http protocol error
 * @throws ResponseException in case Elasticsearch responded with a status code that indicated an error
 */
- public Response performRequest(Request request) throws IOException {
+ public Response performRequest(Request request, RequestOptions options) throws IOException {


I would try not to touch these signatures. The idea could be to make Options a subclass of Request, and also an instance member of Request, so that the low-level REST client's methods remain the same, only to way to set the options changes (we haven't released yet the new request flavoured methods anyways).

I could make a Request.setOptions method but I don't think it makes sense for Request to extend RequestOptions or the other way around.

Agreed I didn't suggest that I meant that Request can hold an instance of Options.

dakrone · 2018-05-22T19:22:00Z

client/rest/src/main/java/org/elasticsearch/client/RequestOptions.java

+ /**
+ * Build the {@linkplain RequestOptions}.
+ */
+ public RequestOptions builder() {


I think this should be build() not builder()

dakrone · 2018-05-22T19:23:24Z

client/rest/src/main/java/org/elasticsearch/client/RequestOptions.java

+ /**
+ * Set the headers to attach to the request.
+ */
+ public void setHeaders(Header... headers) {


Since this is a builder, I do think it'd be nice to be able to add headers iteratively, with something like

builder.addHeader("X-Header", "foo"); builder.addHeader("User-Agent", "bob");

I'd say that's the plan given that we just made the same change to the existing Request.

javanna · 2018-05-28T12:22:18Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ESClientYamlSuiteTestCase.java

@@ -322,7 +322,6 @@ public void test() throws IOException {
 if (useDefaultNumberOfShards == false
 && testCandidate.getTestSection().getSkipSection().getFeatures().contains("default_shards") == false) {
 final Request request = new Request("PUT", "/_template/global");
- request.addHeader("Content-Type", XContentType.JSON.mediaTypeWithoutParameters());


isn't this header required?

I believe we get it by default by setting the entity with json.

sure but then you need to call setJsonEntity below? I would hope that this is going to break tests

Hmmm. It doesn't seem to. Let me poke it a bit more.

Good call on this one. This code path is used rarely(). I got it to execute and it failed. I switched to setJsonEntity and it worked again. I like that better than setting the header. I'll push a fix soon.

I like it better this way but there is no reason to change it. I had changed it because I added something to the top and then removed it.

They are no longer required.

nik9000 · 2018-05-29T15:19:38Z

@javanna I think this is ready for you to review again! I haven't run all the tests locally but I've run the ones that I think will fail and Jenkins will run them all for me.

javanna

left some comments but no blockers. I guess that we can extend this to all the other methods. Note that for methods that have not been released yet, we may want to just replace them instead of deprecating the old variant.

javanna · 2018-05-30T12:12:47Z

client/rest/src/main/java/org/elasticsearch/client/Request.java

@@ -127,40 +119,32 @@ public HttpEntity getEntity() {
 }

 /**
- * Add the provided header to the request.
+ * Set the portion the configuration of an HTTP request to


Set the configuration portion?

javanna · 2018-05-30T12:13:12Z

client/rest/src/main/java/org/elasticsearch/client/Request.java

 }

 /**
- * Headers to attach to the request.
+ * Set the portion the configuration of an HTTP request to


javanna · 2018-05-30T12:13:54Z

client/rest/src/main/java/org/elasticsearch/client/Request.java

- * {@link HttpAsyncResponseConsumer} callback per retry. Controls how the
- * response body gets streamed from a non-blocking HTTP connection on the
- * client side.
+ * gET the portion the configuration of an HTTP request to


javanna · 2018-05-30T12:41:46Z

...t/rest-high-level/src/test/java/org/elasticsearch/client/CustomRestHighLevelClientTests.java

 .when(restClient)
 .performRequest(any(Request.class));

 doAnswer(inv -> mockPerformRequestAsync(
- ((Request) inv.getArguments()[0]).getHeaders().iterator().next(),
+ ((Request) inv.getArguments()[0]),


can you fix the javadocs link for mockPerformRequestAsync ?

javanna · 2018-05-30T12:42:02Z

...t/rest-high-level/src/test/java/org/elasticsearch/client/CustomRestHighLevelClientTests.java

+ assertEquals(nodeName, future.get().getNodeName());
+ }
+
+ private RequestOptions optionsForNodeName(String nodeName) {


javanna · 2018-05-30T12:42:13Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java

@@ -746,6 +799,15 @@ protected final ElasticsearchStatusException parseResponseException(ResponseExce
 }
 }

+ private RequestOptions optionsForHeaders(Header[] headers) {


javanna · 2018-05-30T12:43:59Z

client/rest/src/main/java/org/elasticsearch/client/RequestOptions.java

+import java.util.ArrayList;
+
+/**
+ * Portion the configuration of an HTTP request to Elasticsearch that


Is "Portion the configuration" what you meant to type?

No, I dropped out a few words there. Fixing.

javanna · 2018-05-30T12:46:44Z

client/rest/src/main/java/org/elasticsearch/client/RequestOptions.java

+ /**
+ * Custom implementation of {@link BasicHeader} that overrides equals and hashCode.
+ */
+ @SuppressWarnings("serial")


what do we need this for?

I'll drop it. Eclipse warns about not having java serialization stuff for the class but we ignore that warning everywhere else.

javanna · 2018-05-30T12:49:26Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ESClientYamlSuiteTestCase.java

@@ -322,7 +322,6 @@ public void test() throws IOException {
 if (useDefaultNumberOfShards == false
 && testCandidate.getTestSection().getSkipSection().getFeatures().contains("default_shards") == false) {
 final Request request = new Request("PUT", "/_template/global");
- request.addHeader("Content-Type", XContentType.JSON.mediaTypeWithoutParameters());


sure but then you need to call setJsonEntity below? I would hope that this is going to break tests

javanna · 2018-05-30T12:51:08Z

x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java

@@ -319,7 +320,10 @@ private void expectBadRequest(CheckedSupplier<Map<String, Object>, Exception> co
 request.addParameter("mode", mode); // JDBC or PLAIN mode
 }
 if (randomBoolean()) {
- request.addHeader("Accept", randomFrom("*/*", "application/json"));
+ // JSON is the default but randomly set it sometime for extra coverage
+ RequestOptions.Builder options = request.getOptions().toBuilder();


out of curiosity: here and in other tests why do we need to get the options and then modify them? Aren't those the default options? I would rather build new options and set them.

They are the default options. I could do RequestOptions.DEFAULTS.toBuilder() just as well but this way feels a little more natural to me.

I see. what happens if users forget to provide e.g. the consumer factory? I guess that is what would happen by building new options?

I don't 100% understand your question but I think the answer is yes. The request.getOptions().toBuilder() means that if they already have something in the options then they are just modifying them.

I think it'd be worth me adding some more to the docs about these options. An example of making one that you share and something to make it more obvious that you should set these options in one go rather than making two builders. I mean, you can make two builder if you want, but it'll involve a couple of extra copy operations.

my question was not clear. I thought there was a way to start with a new builder without providing e.g. the consumer factory and set some header. In that case I was wondering what the client would do. What I was getting to was "would it be possible to do without this get default, modify and set, rather just build new ones and set". we could have the default consumer factory as a default in the builder in case it's not set? and the headers would be empty by default anyway? Maybe I am missing something though :)

I did it this way because "build new ones" amounts to the same thing as "create a builder from the DEFAULT instance". We want people to think of this as share-able so I made the API for building new ones always start with a shared "default" one.

javanna · 2018-05-30T13:32:24Z

client/rest/src/main/java/org/elasticsearch/client/RequestOptions.java

+
+ private Builder(List<Header> headers, HttpAsyncResponseConsumerFactory httpAsyncResponseConsumerFactory) {
+ this.headers = new ArrayList<>(headers);
+ this.httpAsyncResponseConsumerFactory = httpAsyncResponseConsumerFactory;


why make this also settable if it's provided at construction time? Also should we check that it's not null?

The idea is that you can make a RequestOptions that you use for all requests and then tweak it for certain requests. So the builder has to start with whatever was in the RequestOptions that you call toBuilder on but you can modify it from there.

gotcha, shall we also have an empty constructor for cases where users want to start from scratch?

I was doing that with RequestOptions.DEFAULTS.toBuilder(). I kind of like how explicit it is about starting with the defaults.

I see, I wonder if having new RequestOptions.Builder() or RequestOptions.builder() would make this simpler from a users perspective.

I think RequestOptions.DEFAULTS.toBuilder() is more obvious when you read it than RequestOptions.builder().

nik9000 · 2018-05-30T17:35:46Z

@elasticmachine, retest this please

This modifies the high level rest client to allow calling code to customize per request options for the bulk API. You do the actual customization by passing a `RequestOptions` object to the API call which is set on the `Request` that is generated by the high level client. It also makes the `RequestOptions` a thing in the low level rest client. For now that just means you use it to customize the headers and the `httpAsyncResponseConsumerFactory` and we'll add node selectors and per request timeouts in a follow up. I only implemented this on the bulk API because it is the first one in the list alphabetically and I wanted to keep the change small enough to review. I'll convert the remaining APIs in a followup.

* 6.x: HLRest: Allow caller to set per request options (#30490) Limit the scope of BouncyCastle dependency (#30959) Deprecates indexing and querying a context completion field without context (#31006) [DOCS] Clarify not all PKCS12 usable as truststores (#30750) Harmonize include_defaults tests (#30700) [DOCS] Update readme for testing x-pack code snippets (#30696) [Docs] Fix typo in Min Aggregation reference (#30899)

* Remove AllocatedPersistentTask.getState() (#30858) This commit removes the method AllocatedPersistentTask.getState() that exposes the internal state of an AllocatedPersistentTask and replaces it with a new isCompleted() method. Related to #29608. * Improve allocation-disabling instructions (#30248) Clarify the “one minute” in the instructions to disable the shard allocation when doing maintenance to say that it is configurable. * Replace several try-finally statements (#30880) This change replaces some existing try-finally statements that close resources in their finally block with the slightly shorter and safer try-with-resources pattern. * Move list tasks under Tasks namespace (#30906) Our API spec define the tasks API as e.g. tasks.list, meaning that they belong to their own namespace. This commit moves them from the cluster namespace to their own namespace. Relates to #29546 * Deprecate accepting malformed requests in stored script API (#28939) The stored scripts API today accepts malformed requests instead of throwing an exception. This PR deprecates accepting malformed put stored script requests (requests not using the official script format). Relates to #27612 * Remove log traces in AzureStorageServiceImpl and fix test (#30924) This commit removes some log traces in AzureStorageServiceImpl and also fixes the AzureStorageServiceTests so that is uses the real implementation to create Azure clients. * Fix IndexTemplateMetaData parsing from xContent (#30917) We failed to register "aliases" and "version" into the list of keywords in the IndexTemplateMetaData; then fail to parse the following index template. ``` { "aliases": {"log": {}}, "index_patterns": ["pattern-1"] } ``` This commit registers that missing keywords. * [DOCS] Reset edit links (#30909) * Limit the scope of BouncyCastle dependency (#30358) Limits the scope of the runtime dependency on BouncyCastle so that it can be eventually removed. * Splits functionality related to reading and generating certificates and keys in two utility classes so that reading certificates and keys doesn't require BouncyCastle. * Implements a class for parsing PEM Encoded key material (which also adds support for reading PKCS8 encoded encrypted private keys). * Removes BouncyCastle dependency for all of our test suites(except for the tests that explicitly test certificate generation) by using pre-generated keys/certificates/keystores. * Upgrade to Lucene-7.4-snapshot-1cbadda4d3 (#30928) This snapshot includes LUCENE-8328 which is needed to stabilize CCR builds. * Moved keyword tokenizer to analysis-common module (#30642) Relates to #23658 * [test] packaging test logging for suse distros * Fix location of AbstractHttpServerTransport (#30888) Currently AbstractHttpServerTransport is in a netty4 module. This is the incorrect location. This commit moves it out of netty4 module. Additionally, it moves unit tests that test AbstractHttpServerTransport logic to server. * [test] packaging: use shell when running commands (#30852) When subprocesses are started with ProcessBuilder, they're forked by the java process directly rather than from a shell, which can be surprising for our use case here in the packaging tests which is similar to scripting. This commit changes the tests to run their subprocess commands in a shell, using the bash -c <script> syntax for commands on linux and using the powershell.exe -Command <script> syntax for commands on windows. This syntax on windows is essentially what the tests were already doing. * [DOCS] Adds missing TLS settings for auditing (#30822) * stable filemode for zip distributions (#30854) Applies default file and directory permissions to zip distributions similar to how they're set for the tar distributions. Previously zip distributions would retain permissions they had on the build host's working tree, which could vary depending on its umask For #30799 * Minor clean-up in InternalRange. (#30886) * Make sure all instance variables are final. * Make generateKey a private static method, instead of protected. * Rename formatter -> format for consistency. * Serialize bucket keys as strings as opposed to optional strings. * Pull the stream serialization logic for buckets into the Bucket class. * [DOCS] Remove reference to platinum Docker image (#30916) * Use dedicated ML APIs in tests (#30941) ML has dedicated APIs for datafeeds and jobs yet base test classes and some tests were relying on the cluster state for this state. This commit removes this usage in favor of using the dedicated endpoints. * Update the version checks around range bucket keys, now that the change was backported. * [DOCS] Fix watcher file location * Rename methods in PersistentTasksService (#30837) This commit renames methods in the PersistentTasksService, to make obvious that the methods send requests in order to change the state of persistent tasks. Relates to #29608. * Rename index_prefix to index_prefixes (#30932) This commit also adds index_prefixes tests to TextFieldMapperTests to ensure that cloning and wire-serialization work correctly * Add missing_bucket option in the composite agg (#29465) This change adds a new option to the composite aggregation named `missing_bucket`. This option can be set by source and dictates whether documents without a value for the source should be ignored. When set to true, documents without a value for a field emits an explicit `null` value which is then added in the composite bucket. The `missing` option that allows to set an explicit value (instead of `null`) is deprecated in this change and will be removed in a follow up (only in 7.x). This commit also changes how the big arrays are allocated, instead of reserving the provided `size` for all sources they are created with a small intial size and they grow depending on the number of buckets created by the aggregation: Closes #29380 * Fsync state file before exposing it (#30929) With multiple data paths, we write the state files for index metadata to all data paths. We only properly fsync on the first location, though. For other locations, we possibly expose the file before its contents is properly fsynced. This can lead to situations where, after a crash, and where the first data path is not available anymore, ES will see a partially-written state file, preventing the node to start up. * Fix AliasMetaData parsing (#30866) AliasMetaData should be parsed more leniently so that the high-level REST client can support forward compatibility on it. This commit addresses this issue that was found as part of #28799 and adds dedicated XContent tests as well. * Cross Cluster Search: do not use dedicated masters as gateways (#30926) When we are connecting to a remote cluster we should never select dedicated master nodes as gateway nodes, or we will end up loading them with requests that should rather go to other type of nodes e.g. data nodes or coord_only nodes. This commit adds the selection based on the node role, to the existing selection based on version and potential node attributes. Closes #30687 * Fix missing option serialization after backport Relates #29465 * REST high-level client: add synced flush API (2) (#30650) Adds the synced flush API to the high level REST client. Relates to #27205. * Fix synced flush docs They had some copy and paste errors that failed the docs build. * Change ScriptException status to 400 (bad request) (#30861) Currently failures to compile a script usually lead to a ScriptException, which inherits the 500 INTERNAL_SERVER_ERROR from ElasticsearchException if it does not contain another root cause. Instead, this should be a 400 Bad Request error. This PR changes this more generally for script compilation errors by changing ScriptException to return 400 (bad request) as status code. Closes #12315 * Fix composite agg serialization error Fix serialization after backport Relates #29465 * Revert accidentally pushed changes in NoriAnalysisTests * SQL: Remove log4j and joda from JDBC dependencies (#30938) More cleanup of JDBC driver project Relates to #29856 * [DOCS] Fixes kibana security file location * [CI] Mute SamlAuthenticatorTests testIncorrectSigningKeyIsRejected Tracked by #30970 * Add Verify Repository High Level REST API (#30934) This commit adds Verify Repository, the associated docs and tests for the high level REST API client. A few small changes to the Verify Repository Response went into the commit as well. Relates #27205 * Add “took” timing info to response for _msearch/template API (#30961) Add “took” timing info to response for _msearch/template API Closes #30957 * Mute FlushIT tests We have identified the source causing these tests failed. This commit mutes them again until we have a proper fix. Relates #29392 * [CI] Mute HttpSecretsIntegrationTests#testWebhookAction test Tracked by #30094 * [Test] Prefer ArrayList over Vector (#30965) Replaces some occurances of Vector class with ArrayList in tests of the rank-eval module. * Fix license on AcitveDirectorySIDUtil (#30972) This code is from an Apache 2.0 licensed codebase and when we imported it into our codebase it carried the Apache 2.0 license as well. However, during the migration of the X-Pack codebase from the internal private repository to the elastic/elasticsearch repository, the migration tool mistakently changed the license on this source file from the Apache 2.0 license to the Elastic license. This commit addresses this mistake by reapplying the Apache 2.0 license. * [CI] Mute Ml rolling upgrade tests Tracked by #30982 * Make AllocatedPersistentTask.isCompleted() protected (#30949) This commit changes the isCompleted() method to be protected so that classes that extends AllocatedPersistentTask can use it. Related to #30858 * [CI] Mute Ml rolling upgrade test for mixed cluster too It can fail in either the mixed cluster or the upgraded cluster, so it needs to be muted in both. Tracked by #30982 * [Docs] Fix typo in Min Aggregation reference (#30899) * Refactor Sniffer and make it testable (#29638) This commit reworks the Sniffer component to simplify it and make it possible to test it. In particular, it no longer takes out the host that failed when sniffing on failure, but rather relies on whatever the cluster returns. This is the result of some valid comments from #27985. Taking out one single host is too naive, hard to test and debug. A new Scheduler abstraction is introduced to abstract the tasks scheduling away and make it possible to plug in any test implementation and take out timing aspects when testing. Concurrency aspects have also been improved, synchronized methods are no longer required. At the same time, we were able to take #27697 and #25701 into account and fix them, especially now that we can more easily add tests. Last but not least, unit tests are added for the Sniffer component, long overdue. Closes #27697 Closes #25701 * Deprecates indexing and querying a context completion field without context (#30712) This change deprecates completion queries and documents without context that target a context enabled completion field. Querying without context degrades the search performance considerably (even when the number of indexed contexts is low). This commit targets master but the deprecation will take place in 6.x and the functionality will be removed in 7 in a follow up. Closes #29222 * Core: Remove RequestBuilder from Action (#30966) This commit removes the RequestBuilder generic type from Action. It was needed to be used by the newRequest method, which in turn was used by client.prepareExecute. Both of these methods are now removed, along with the existing users of prepareExecute constructing the appropriate builder directly. * Ensure intended key is selected in SamlAuthenticatorTests (#30993) * Ensure that a purposefully wrong key is used Uses a specific keypair for tests that require a purposefully wrong keypair instead of selecting one randomly from the same pull from which the correct one is selected. Entropy is low because of the small space and the same key can be randomly selected as both the correct one and the wrong one, causing the tests to fail. The purposefully wrong key is also used in testSigningKeyIsReloadedForEachRequest and needs to be cleaned up afterwards so the rest of the tests don't use that for signing. Resolves #30970 * [DOCS] Update readme for testing x-pack code snippets (#30696) * Remove version read/write logic in Verify Response (#30879) Since master will always communicate with a >=6.4 node, the logic for checking if the node is 6.4 and conditionally reading and writing based on that can be removed from master. This logic will stay in 6.x as it is the bridge to the cleaner response in master. This also unmutes the failing test due to this bwc change. Closes #30807 * HLRest: Allow caller to set per request options (#30490) This modifies the high level rest client to allow calling code to customize per request options for the bulk API. You do the actual customization by passing a `RequestOptions` object to the API call which is set on the `Request` that is generated by the high level client. It also makes the `RequestOptions` a thing in the low level rest client. For now that just means you use it to customize the headers and the `httpAsyncResponseConsumerFactory` and we'll add node selectors and per request timeouts in a follow up. I only implemented this on the bulk API because it is the first one in the list alphabetically and I wanted to keep the change small enough to review. I'll convert the remaining APIs in a followup. * [DOCS] Clarify not all PKCS12 usable as truststores (#30750) Although elasticsearch-certutil generates PKCS#12 files which are usable as both keystore and truststore this is uncommon in practice. Settle these expectations for the users following our security guides. * Transport client: Don't validate node in handshake (#30737) This is related to #30141. Right now in the transport client we open a temporary node connection and take the node information. This node information is used to open a permanent connection that is used for the client. However, we continue to use the configured transport address. If the configured transport address is a load balancer, you might connect to a different node for the permanent connection. This causes the handshake validation to fail. This commit removes the handshake validation for the transport client when it simple node sample mode. * Remove unused query methods from MappedFieldType. (#30987) * Remove MappedFieldType#nullValueQuery, as it is now unused. * Remove MappedFieldType#queryStringTermQuery, as it is never overridden. * Reuse expiration date of trial licenses (#30950) * Retain the expiryDate for trial licenses While updating the license signature to the new license spec retain the trial license expiration date to that of the existing license. Resolves #30882 * Watcher: Give test a little more time Changes watcher's integration tests to wait 30 seconds when starting watcher rather than 10 seconds because this build failed when starting took 12 seconds: https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+6.3+periodic/222/console

* master: Avoid randomization bug in FeatureAwareTests Adjust BWC version on client features Add TRACE, CONNECT, and PATCH http methods (#31035) Adjust BWC version on client features [DOCS] Make geoshape docs less memory hungry (#31014) Fix handling of percent-encoded spaces in Windows batch files (#31034) [Docs] Fix a typo in Create Index naming limitation (#30891) Introduce client feature tracking (#31020) Ensure that index_prefixes settings cannot be changed (#30967) REST high-level client: add delete ingest pipeline API (#30865) [ML][TEST] Fix bucket count assertion in all tests in ModelPlotsIT (#31026) Allow rollup job creation only if cluster is x-pack ready (#30963) Fix interoperability with < 6.3 transport clients (#30971) Add an option to split keyword field on whitespace at query time (#30691) [Tests] Fix alias names in PutIndexTemplateRequestTests (#30960) REST high-level client: add get ingest pipeline API (#30847) Cross Cluster Search: preserve remote status code (#30976) High-level client: list tasks failure to not lose nodeId (#31001) [DOCS] Fixes links (#31011) Watcher: Give test a little more time Reuse expiration date of trial licenses (#30950) Remove unused query methods from MappedFieldType. (#30987) Transport client: Don't validate node in handshake (#30737) [DOCS] Clarify not all PKCS12 usable as truststores (#30750) HLRest: Allow caller to set per request options (#30490) Remove version read/write logic in Verify Response (#30879) [DOCS] Update readme for testing x-pack code snippets (#30696) Ensure intended key is selected in SamlAuthenticatorTests (#30993) Core: Remove RequestBuilder from Action (#30966)

With elastic#30490 we have introduced a new way to provide request options whenever sending a request using the high-level REST client. Before you could provide headers as the last argument varargs of each API method, now you can provide `RequestOptions` that in the future will allow to provide more options which can be specified per request. This commit deprecates all of the client methods that accept a `Header` varargs argument in favour of new methods that accept `RequestOptions` instead. For some API we don't even go through deprecation given that they were not released since they were added, hence in that case we can just move them to the new method.

With #30490 we have introduced a new way to provide request options whenever sending a request using the high-level REST client. Before you could provide headers as the last argument varargs of each API method, now you can provide `RequestOptions` that in the future will allow to provide more options which can be specified per request. This commit deprecates all of the client methods that accept a `Header` varargs argument in favour of new methods that accept `RequestOptions` instead. For some API we don't even go through deprecation given that they were not released since they were added, hence in that case we can just move them to the new method.

nik9000 added 5 commits May 8, 2018 18:32

WIP

1964066

WIP

647f4ca

Merge branch 'master' into rest_hl_request

d2f56a2

Fix

2642f01

nik9000 added >enhancement review v7.0.0 v6.4.0 labels May 9, 2018

nik9000 requested a review from javanna May 9, 2018 20:41

nik9000 added 3 commits May 18, 2018 16:02

Merge branch 'master' into rest_hl_request

fad1513

Merge branch 'master' into rest_hl_request

c9f2320

Start adding a RequestOptions

afb800f

javanna reviewed May 22, 2018

View reviewed changes

dakrone reviewed May 22, 2018

View reviewed changes

nik9000 added 3 commits May 25, 2018 13:53

Merge branch 'master' into rest_hl_request

4f2bd82

Further

5330e72

Merge branch 'master' into rest_hl_request

07c3bd5

javanna reviewed May 28, 2018

View reviewed changes

Fix some more

d1a72a4

nik9000 added 4 commits May 29, 2018 11:07

Remove unused import

f64db06

Revert newline change

f83454c

I like it better this way but there is no reason to change it. I had changed it because I added something to the top and then removed it.

Remove overloads suppression

0cb6502

They are no longer required.

Add options to toString

bb4f6f3

Merge branch 'master' into rest_hl_request

f4630b5

javanna approved these changes May 30, 2018

View reviewed changes

javanna reviewed May 30, 2018

View reviewed changes

nik9000 added 3 commits May 30, 2018 10:04

Fix mistake

c1b3321

Cleanup

364913e

Merge branch 'master' into rest_hl_request

74be65c

nik9000 merged commit b225f5e into elastic:master May 31, 2018

This was referenced Jun 4, 2018

Add high-level client methods that accept RequestOptions #31069

Merged

high level REST api: cancel task #30745

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

HLRest: Allow caller to set per request options #30490

HLRest: Allow caller to set per request options #30490

Conversation

nik9000 commented May 9, 2018 • edited Loading

elasticmachine commented May 9, 2018

javanna commented May 11, 2018 • edited Loading

nik9000 commented May 11, 2018

javanna commented May 11, 2018

tlrx commented May 14, 2018

dakrone commented May 15, 2018

cbuescher commented May 16, 2018

nik9000 commented May 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented May 29, 2018

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented May 30, 2018

nik9000 commented May 9, 2018 •

edited

Loading

javanna commented May 11, 2018 •

edited

Loading