Rest High Level client: Add List Tasks #29546

Van0SS · 2018-04-16T22:19:04Z

Related to #27205

elasticmachine · 2018-04-17T08:20:40Z

Pinging @elastic/es-core-infra

cbuescher · 2018-04-18T15:40:05Z

cbuescher

Hi @Van0SS, thanks for your help on the client. This PR aready looks quiet good, I left a couple of questions I had while reviewing it, especially around the need to implement equals/hashCode for some response and failure classes. If we want to keep that then some of the tests might need some updates to test those more thorougly. But as far as the rest goes I'm okay with most of it.
If you could merge in master and fix the two long lines, then I can also kick off another CI test run while working on the rest, so we can see if all test pass.

cbuescher · 2018-04-18T15:51:54Z

.../src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/list/ListTasksResponse.java

+                int i = 0;
+                @SuppressWarnings("unchecked") List<TaskInfo> tasks = (List<TaskInfo>) constructingObjects[i++];
+                @SuppressWarnings("unchecked") List<TaskOperationFailure> tasksFailures = (List<TaskOperationFailure>) constructingObjects[i++];
+                @SuppressWarnings("unchecked") List<ElasticsearchException> nodeFailures = (List<ElasticsearchException>) constructingObjects[i];


nit: checkstyle is a bit picky and requires the two lines above to be less than 140, could you add line breaks somewhere, e.g. put the supressions in separate lines?

cbuescher · 2018-04-18T16:03:55Z

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

+            .withParentTaskId(request.getParentTaskId())
+            .withNodes(request.getNodes())
+            .withActions(request.getActions())
+            .putParam("group_by", "none");


For my understanding: is "group_by" fixed here because the current implementation of ListTasksRequests doesn't prrovide it (I had a quick look and couldn't find anything)? If so, should we support it? When I look at the docs it seems like group_by=parents is supported.

To parse the response we only need group_by=none. Grouping by parent or nodes will be done on the client side when you call getTaskGroups() or getPerNodeTasks() in ListTasksResponse.
But, by default API returns group_by=parent result, so I added here specific parameter.

Thanks for clearing this up for me, makes sense.

cbuescher · 2018-04-18T16:22:38Z

server/src/main/java/org/elasticsearch/action/TaskOperationFailure.java

@@ -95,16 +118,20 @@ public Exception getCause() {

    @Override
    public String toString() {
-        return "[" + nodeId + "][" + taskId + "] failed, reason [" + getReason() + "]";


While clients of this class shouldn't rely on the format of this string representation, we should double check that changing it to json format doesn't break anything. If in doubt I'd probably keep it as is, but open to discussion. I will also dig a bit more if we rely on this format internally.

I did not find directly usages of TaskOperationFailure::toString and for me seems that nobody should rely on toString until it's specifically mentioned in JavaDoc for this method, maybe I live in a parallel universe :)

No, I agree. Nevertheless we might want to play it save, but I will get another opinion on this.

I think that this change is not necessary, and changing it is a risk. It could just be that this is logged somewhere and the current toString output makes sense the way it is, maybe even more readable than the json output.

cbuescher · 2018-04-18T16:27:23Z

server/src/main/java/org/elasticsearch/action/TaskOperationFailure.java

@@ -113,4 +140,19 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws

    }

+    @Override
+    public boolean equals(Object o) {


Do we need equals/hashCode here? Might be its only added to be used in test, in that case I'd take another look if we really need it, especially since "reason" isn't checked. Semantically I'm not sure if two failures with same TaskId, nodeId, status should be considered "equal", e.g. when inserted into a set (which we probably don't so, but nethertheless). Not something I absolutely disagree with but maybe worth another look.

cbuescher · 2018-04-18T16:35:42Z

.../src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/list/ListTasksResponse.java

@@ -56,11 +67,28 @@ public ListTasksResponse() {
    }

    public ListTasksResponse(List<TaskInfo> tasks, List<TaskOperationFailure> taskFailures,
-            List<? extends FailedNodeException> nodeFailures) {
+            List<? extends ElasticsearchException> nodeFailures) {


Maybe we could keep the public ctor using the more restricted FailedNodeException and have another private ctor that the parser can use that takes a list of ElasticsearchException? Maybe not worth the trouble though, just thinking out loud.

Nevermind, reading on I think I see why you need this...

cbuescher · 2018-04-18T16:45:06Z

server/src/main/java/org/elasticsearch/action/support/tasks/BaseTasksResponse.java

@@ -42,9 +42,9 @@
 */
 public class BaseTasksResponse extends ActionResponse {
    private List<TaskOperationFailure> taskFailures;
-    private List<FailedNodeException> nodeFailures;
+    private List<ElasticsearchException> nodeFailures;


I assume this is necessary because we cannot parse back to FailedNodeException?

cbuescher · 2018-04-18T16:47:42Z

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestListTasksAction.java

@@ -103,18 +103,17 @@ public RestResponse buildResponse(T response, XContentBuilder builder) throws Ex
                    return new BytesRestResponse(RestStatus.OK, builder);
                }
            };
-        } else if ("none".equals(groupBy)) {
+        } else if ("parents".equals(groupBy)) {


Why the switch here? I'm probably missing some context.

Parser for ListTasksResponse uses only groupby=node response format, so better toXContent also serialize to groupby=node format. It's not necessarily but a nice thing as it seems for me.

So this changes the structure of the Rest response output? In this case we shouldn't do it for backward compatiblity reasons. This change should also be merged to the 6.x branches I think.

It won't change REST output in the REST endpoint default is still groupby=nodes
See RestListTasksAction::prepareRequest :
String groupBy = request.param("group_by", "nodes");

Yes, for the requests coming though the client I see this, what about requests that set groupby differently themselves? I see you also changes ListTaskResponse#toXContent to use toXContentGroupedByNone instead of toXContentGroupedByParents, I think thats why this switch here doesn't change anything but I'd still like to understand the reason for that change better.

I am sorry, I thought all this conversation was about not only this 2 lines of code switching 2 parameters in if clause, but about all the changes including ListTaskResponse#toXContent to use toXContentGroupedByNone instead of toXContentGroupedByParents.
So all these changes don't change REST endpoint behavior. It just changes toXContent result to be groupby=none, it's not necessarily, but can bring:

Idiomatically correct serialization/deserialization workflow: toXContent returns content which can be deserialized in fromXContent

Tests can use Serialization handlers like XContentHelper::toXContent or can be fully automated i.e. extend AbstractStreamableXContentTestCase or AbstractSerializingTestCase (Currently not applicable because of broken equals)

And again it just a cosmetic change and I am happy to revert it back :)

Thanks for clearing this up, I'll just briefly summarize to see if I understand everything correctly:

ListTaskResponse#toXContent has been changed to use toXContentGroupedByNone instead of toXContentGroupedByParents to make it output the same structure that the fromXContent method parses.

this doesn't change the default rest response because RestListTasksAction::prepareRequest sets it to "nodes (String groupBy = request.param("group_by", "nodes")) and in the client code added in this PR you set it to "groupeBy=none" explicitely (see above)

The actual decision which output format is rendered isn't determined in ListTaskResponse but here by the listener that is chosen in RestListTasksAction::listTasksResponseListener

This all looks good to me now that I wrapped my head around it. I still wonder if it is possible that somebody using the transport client today might use the "toXContent()" method directly on the response and would be suprised by the changed output and where this puts us in terms of marking this as breaking or not, but it shouldn't matter for the REST Api.

cbuescher · 2018-04-18T16:58:16Z

server/src/test/java/org/elasticsearch/action/TaskOperationFailureTests.java

+
+import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent;
+
+public class TaskOperationFailureTests extends AbstractWireSerializingTestCase<TaskOperationFailure> {


Its not strictly enforced yet, but if extending AbstractWireSerializingTestCase there is a mutateInstance method from AbstractWireTestCase that ideally should be implemented to get meaningfull test for hashCode/equals (otherwise the instance compared to is always null). Together with my previous comment that I'm not sure TaskOperationFailure should support equals/hashCode, it might be possible to extend another base test.

cbuescher · 2018-04-18T17:02:47Z

server/src/test/java/org/elasticsearch/tasks/ListTasksResponseTests.java


-public class ListTasksResponseTests extends ESTestCase {
+public class ListTasksResponseTests extends AbstractStreamableTestCase<ListTasksResponse> {


Same as above, this should probably get a mutateInstance method as well that changes each aspect of the response randomly (only one property at once) to get meaningfull equals/hashCode testing. Then again I'm on the fence if we should implement equals/hashCode here at all.

I think it's generally good to try and extend AbstractStreamableTestCase, but in this case it's a bit of a stretch as the response holds a failure which holds an exception which we cannot parse back exactly with the same type. That affects the equality comparisons as we need them only in tests, and they make sense only for our tests. I also suspect that the equality comparisons needed for the xcontent tests are different to the ones needed for wire serialization tests, unless exceptions are simply omitted.

I would rather look into extending AbstractXContentTestsCase here and in all this PR tests which allows to override assertEqualInstances so that we don't need to implement equals/hashcode.

If we also wanted to add serialization tests, which is not required here though, and could be done as a follow-up, we could create a different test class that extends AbstractWireTestCase to test only wire serialization. That one allows to override assertEqualInstances too

cbuescher · 2018-04-18T17:11:38Z

server/src/test/java/org/elasticsearch/tasks/TaskInfoTests.java

+import java.util.Map;
+import java.util.function.Predicate;
+
+public class TaskInfoTests extends AbstractSerializingTestCase<TaskInfo> {


This test could also use a mutateInstance method overwriting the trivial default implementation from AbstractWireTestCase. In this case it might make sense since the TaskInfo equals/hashCode is quiet robust. Unfortunately there are quiet a number of parameters that can be altered, so its might be a bit of work. You can take a look at other tests that implement this how ppl usually approach it.

Agree, will add mutateInstance

…o enhancement/rest_hl_client_cluster_list_tasks

Van0SS · 2018-04-18T18:04:58Z

@cbuescher thanks for the review! I fixed checkstyle and merged to master, you can try to run build again.

Van0SS · 2018-04-18T18:11:04Z

@cbuescher I am also on the fence about equals/hashcode for Classes which have Exceptions. Will wait what do you guys decide. If decision will be to use them then I will added mutations to tests.

cbuescher · 2018-04-18T19:49:28Z

@elasticsearch test this please

cbuescher · 2018-04-18T19:55:06Z

@Van0SS thanks a lot, looks great. I left some comments, will try to get some feedback on the open questions. In the meantime I kicked off some full test runs, although the PR might still change but just in case this might catch anything unexpected.

cbuescher · 2018-04-18T19:56:07Z

Never mind the failing packaging tests, I think that's a temporary issue we have since we updated version constants on master a few hours ago. I should resolve itself once merging master again.

cbuescher · 2018-04-20T15:00:39Z

.../src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/list/ListTasksResponse.java

@@ -204,14 +235,14 @@ public XContentBuilder toXContentGroupedByNone(XContentBuilder builder, Params p
    @Override
    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
        builder.startObject();
-        toXContentGroupedByParents(builder, params);
+        toXContentGroupedByNone(builder, params);


I overlooked this in my first round of reviews, I'd like to undestand why this needs changing.

as far as I understand this makes parsing easier, yet there is the concern that it may break existing transport client users calling toXContent on the obtained response object, given that the method is public. I wonder if we could add parsing without this slight breakage and how much work that would be. It is odd that we need this behaviour change technically only to make our tests work, given that fromXContent knows to parse only the groupedByNone output. What options do we have?

make fromXContent parse the groupedByParents output instead?

make it possible in some way to call toXContentGroupedByNone in our tests without changing the default behaviour of toXContent. This may be possible by making the output depend on a param passed using the params argument. The test would also have to set that param, which is not supported at the moment. This sounds rather complicated, not sure it' worth it

Accept this slight breaking change

Other alternatives?

@javanna I hear you, if non-breaking changes is that important then better find solution without it.

Another alternative - don't change default serialization and use custom testcase. Like it is now in this PR in ListTasksResponseTests::testXContentWithFailures().
So, in this testcase we can explicitly call ListTasksResponse::toXContentGroupedByNone, parse, do object comparison, etc.
Not the best solution also though.

Could we move toXContent to look at params and call the proper method depending on that? Could the REST action call that instead of the custom rest builder listeners? Then we could reuse your test changes from the cluster health PR, where you can define ToXContent.Params through a new protected method and provide the one that we need in the test? Not sure if I am missing something, I haven't tried this out but this could be the way to go.

@javanna It is nice idea, but I stuck with implementation - toXContentGroupedByNode requires DiscoveryNodes parameter, which cannot be passed as Params bc Params can store only String values.
We can add paramAsObject and add implementation which can store any Object but not sure that it is a good idea. What do you think?

I see the problem @Van0SS thanks for bringing that up. I am thinking that it's not wort to refactor this for the corner case that we are worrying about.

@javanna so, I will change none and parents to use toXContent and Params parameter, but nodes still will be using toXContentGroupedByNode. Is it what we want to implement?

@Van0SS I was actually thinking that no change is required if we accept the minor breakage described above. Agreed?

@javanna Absolutely :)

I just checked that we have a yaml rest test that makes sure we are not breaking the default rest response here (which will still be grouping by node).
So to summarize: this will only change the output for users that use ListTasksResponse#toXContent directly, which we accept because is makes parsing/toXContent compatibly and users shouldn't rely on xContent output format on this level. @javanna am I correct with this and do you agree? What does this mean for labeling this issue and migration docs for backporting to 6.4? Do you agree we can ignore this?

cbuescher

@Van0SS sorry for the delay, I finally got around asking @javanna for another opinion on the open topics, we agreed on the following:

prefer to leave "TaskOperationFailure#toString" as is, since its not strictly needed for this PR and it might be used in logging somewhere where the json format is not ideal
not implementing equals/hashCode for TaskOperationFailure and ListTaskResponse for the reasons stated. Also that means we don't need to test it, but you might need to find a way to work around the use of equals() for serialization/parsing tests in the base tests. See comments and let me know if there are any questions
need to investigate the options for changing the ListTaskResponse#toXContent output. While I prefer the change the way you do it, there is a marginal possibility that some user relies on the current behaviour, making this a breaking Java change. This might be acceptable, but maybe there are alternatives.

Let me know what you think, again other than that this is very close to be ready.

cbuescher · 2018-05-03T10:38:52Z

It also looks like there are some changes that need to be merged in from master.

javanna · 2018-05-11T08:05:22Z

hi @Van0SS would you have time to address the latest review comments? Also you'll need to merge master in and fix conflicts, for the most part Request is now called RequestConverters. Let us know if we can help.

Van0SS · 2018-05-12T16:53:17Z

@javanna Sorry didn't have time recently, will try to make changes soon. Thanks for ideas!

Conflicts: client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java docs/java-rest/high-level/supported-apis.asciidoc

cbuescher · 2018-05-14T14:11:28Z

@Van0SS I took the liberty and merged in master to see what is left to be done. I will go over the PR once again and see if there are any other mayor things missing now that @javanna is okay with the ListTasksResponse toXContent change.

cbuescher

Currently the main things to change here are removing the mentioned equals/hashcode implementations and reverting the changes in the toString() method of TaskOperationFailure. Also I think there was some discussion about extending some tests adding the mutate function().
I will still kick off a CI run to see if the rest looks okay already.

cbuescher · 2018-05-14T14:50:18Z

server/src/main/java/org/elasticsearch/action/TaskOperationFailure.java

@@ -95,16 +118,20 @@ public Exception getCause() {

    @Override
    public String toString() {
-        return "[" + nodeId + "][" + taskId + "] failed, reason [" + getReason() + "]";
+        return Strings.toString(this);


Related to the former discussion, would you change this back please?

cbuescher · 2018-05-14T14:55:21Z

@elasticmachine test this please

Van0SS · 2018-05-14T17:00:48Z

@cbuescher Thanks for merging! Working on final changes.

- Remove equals, hashcode, WireSerializing tests - Revert toString() - Add mutateInstance - Use custom assertEqualInstances()

cbuescher · 2018-05-15T08:50:30Z

@Van0SS thanks for the update, will take a look at it soon, just kicking of another test run in the meantime
@elasticmachine test this please

cbuescher

Looks great, I left only a few minor comments, and maybe you can resurrect TaskOperationFailureTests that you removed in the last commit in a slightly different manner that doesn't require hashCode/Equals?

cbuescher · 2018-05-15T10:26:48Z

server/src/main/java/org/elasticsearch/action/TaskOperationFailure.java

@@ -48,6 +56,21 @@

    private final RestStatus status;

+    public static final ConstructingObjectParser<TaskOperationFailure, Void> PARSER =


nit: maybe the parse should be private, where it is needed in ListTaskResponse you could use TaskOperationFailure.fromXContent.

cbuescher · 2018-05-15T10:27:21Z

.../src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/list/ListTasksResponse.java

        super(taskFailures, nodeFailures);
        this.tasks = tasks == null ? Collections.emptyList() : Collections.unmodifiableList(new ArrayList<>(tasks));
    }

+    public static final ConstructingObjectParser<ListTasksResponse, Void> PARSER =


nit: private

cbuescher · 2018-05-15T10:30:06Z

.../src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/list/ListTasksResponse.java

+
+    static {
+        PARSER.declareObjectArray(constructorArg(), TaskInfo.PARSER, new ParseField(TASKS));
+        PARSER.declareObjectArray(optionalConstructorArg(), TaskOperationFailure.PARSER, new ParseField(TASK_FAILURES));


re: suggestion about. You could use (p, c) -> TaskOperationFailure.fromXContent(p) or similar instead of accessing the TaskOperationFailure.PARSER directly

cbuescher · 2018-05-15T10:52:39Z

.../src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/list/ListTasksResponse.java

@@ -204,14 +235,14 @@ public XContentBuilder toXContentGroupedByNone(XContentBuilder builder, Params p
    @Override
    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
        builder.startObject();
-        toXContentGroupedByParents(builder, params);
+        toXContentGroupedByNone(builder, params);


I just checked that we have a yaml rest test that makes sure we are not breaking the default rest response here (which will still be grouping by node).
So to summarize: this will only change the output for users that use ListTasksResponse#toXContent directly, which we accept because is makes parsing/toXContent compatibly and users shouldn't rely on xContent output format on this level. @javanna am I correct with this and do you agree? What does this mean for labeling this issue and migration docs for backporting to 6.4? Do you agree we can ignore this?

cbuescher · 2018-05-15T11:16:17Z

server/src/test/java/org/elasticsearch/action/TaskOperationFailureTests.java

-
-import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent;
-
-public class TaskOperationFailureTests extends AbstractWireSerializingTestCase<TaskOperationFailure> {


I think this is still a useful test to have, just without relying on equals/hashCode. So maybe you could structure it similar to ListTasksResponseTests, e.g. extend AbstractXContentTestCase using assertEqualInstances?

- Restore toXContent test - Make PARSER private - Cleanup imports

Conflicts: client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java docs/java-rest/high-level/supported-apis.asciidoc

cbuescher

LGTM, will start the tests.

cbuescher · 2018-05-16T07:18:45Z

Jenkins, test this please

cbuescher · 2018-05-16T11:36:43Z

@Van0SS finally a clean CI run. Thanks so much for sticking with this through all the discussion about the nitty-gritty bwc details. It's a great PR and a valuable addition to the ongoing effort to make the high level rest client more complete. Much appreciated.
I will also merge this to the current 6.x branch so this should be coming out with 6.4.

This change adds a `listTasks` method to the high level java ClusterClient which allows listing running tasks through the task management API. Related to elastic#27205

Van0SS · 2018-05-16T15:25:29Z

Woohoo! Thank you guys so much for your effort and patience.

This change adds a `listTasks` method to the high level java ClusterClient which allows listing running tasks through the task management API. Related to #27205

…ngs-to-true * elastic/master: (34 commits) Test: increase search logging for LicensingTests Adjust serialization version in IndicesOptions [TEST] Fix compilation Remove version argument in RangeFieldType (elastic#30411) Remove unused DirectoryUtils class. (elastic#30582) Mitigate date histogram slowdowns with non-fixed timezones. (elastic#30534) Add a MovingFunction pipeline aggregation, deprecate MovingAvg agg (elastic#29594) Removes AwaitsFix on IndicesOptionsTests Template upgrades should happen in a system context (elastic#30621) Fix bug in BucketMetrics path traversal (elastic#30632) Fixes IndiceOptionsTests to serialise correctly (elastic#30644) S3 repo plugin populate SettingsFilter (elastic#30652) mute IndicesOptionsTests.testSerialization Rest High Level client: Add List Tasks (elastic#29546) Refactors ClientHelper to combine header logic (elastic#30620) [ML] Wait for ML indices in rolling upgrade tests (elastic#30615) Watcher: Ensure secrets integration tests also run triggered watch (elastic#30478) Move allocation awareness attributes to list setting (elastic#30626) [Docs] Update code snippet in has-child-query.asciidoc (elastic#30510) Replace custom reloadable Key/TrustManager (elastic#30509) ...

* es/6.x: (44 commits) SQL: Remove dependency for server's version from JDBC driver (#30631) Make xpack modules instead of a meta plugin (#30589) Security: Remove SecurityLifecycleService (#30526) Build: Add task interdependencies for ssl configuration (#30633) Mute ShrinkIndexIT [ML] DeleteExpiredDataAction should use client with origin (#30646) Reindex: Fixed typo in assertion failure message (#30619) [DOCS] Fixes list of unconverted snippets in build.gradle Use readFully() to read bytes from CipherInputStream (#30640) Add Create Repository High Level REST API (#30501) [DOCS] Reorganizes RBAC documentation Test: increase search logging for LicensingTests Delay _uid field data deprecation warning (#30651) Deprecate Empty Templates (#30194) Remove unused DirectoryUtils class. (#30582) Mitigate date histogram slowdowns with non-fixed timezones. (#30534) [TEST] Remove AwaitsFix in IndicesOptionsTests#testSerialization S3 repo plugin populates SettingsFilter (#30652) Rest High Level client: Add List Tasks (#29546) Fixes IndiceOptionsTests to serialise correctly (#30644) ...

This change adds a `listTasks` method to the high level java ClusterClient which allows listing running tasks through the task management API. Related to elastic#27205

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 elastic#29546

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

* 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

Rest High Level client: Add List Tasks.

bf0da19

cbuescher added review labels Apr 17, 2018

Van0SS changed the title ~~Rest High Level client: Add List Tasks.~~ Rest High Level client: Add List Tasks Apr 17, 2018

cbuescher self-assigned this Apr 18, 2018

cbuescher requested changes Apr 18, 2018

View reviewed changes

Van0SS added 3 commits April 18, 2018 13:21

Fix checkstyle

405609e

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

419e115

…o enhancement/rest_hl_client_cluster_list_tasks

Fix checkstyle in a better way

5e67c4f

javanna mentioned this pull request Apr 20, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

cbuescher reviewed Apr 20, 2018

View reviewed changes

colings86 added the >feature label Apr 24, 2018

cbuescher requested changes May 3, 2018

View reviewed changes

cbuescher added v7.0.0 v6.4.0 labels May 7, 2018

cbuescher requested changes May 14, 2018

View reviewed changes

Final changes (Hope):

1058521

- Remove equals, hashcode, WireSerializing tests - Revert toString() - Add mutateInstance - Use custom assertEqualInstances()

cbuescher requested changes May 15, 2018

View reviewed changes

Van0SS and others added 2 commits May 15, 2018 14:22

Final nits 2x

796cd2d

- Restore toXContent test - Make PARSER private - Cleanup imports

Merge branch 'master' into enhancement/rest_hl_client_cluster_list_tasks

330c59c

Conflicts: client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java docs/java-rest/high-level/supported-apis.asciidoc

cbuescher approved these changes May 16, 2018

View reviewed changes

cbuescher merged commit 4478f10 into elastic:master May 16, 2018

cbuescher pushed a commit that referenced this pull request May 16, 2018

Rest High Level client: Add List Tasks (#29546)

c4ed1cd

This change adds a `listTasks` method to the high level java ClusterClient which allows listing running tasks through the task management API. Related to #27205

cbuescher mentioned this pull request May 16, 2018

Rest High Level client: Add List Tasks (#29546) #30649

Merged

javanna mentioned this pull request May 28, 2018

Move list tasks API under tasks namespace #30906

Merged

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

davidkyle mentioned this pull request Mar 18, 2019

[ML] Data Frame HLRC start & stop APIs #40154

Merged


		import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent;

		public class TaskOperationFailureTests extends AbstractWireSerializingTestCase<TaskOperationFailure> {


		public class ListTasksResponseTests extends ESTestCase {
		public class ListTasksResponseTests extends AbstractStreamableTestCase<ListTasksResponse> {

		@@ -48,6 +56,21 @@

		private final RestStatus status;

		public static final ConstructingObjectParser<TaskOperationFailure, Void> PARSER =

Rest High Level client: Add List Tasks #29546

Rest High Level client: Add List Tasks #29546

Conversation

Van0SS commented Apr 16, 2018

elasticmachine commented Apr 17, 2018

cbuescher commented Apr 18, 2018

cbuescher 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

Van0SS Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Van0SS Apr 20, 2018 • edited Loading

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

javanna May 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Van0SS Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Van0SS commented Apr 18, 2018

Van0SS commented Apr 18, 2018

cbuescher commented Apr 18, 2018

cbuescher commented Apr 18, 2018

cbuescher commented Apr 18, 2018

Choose a reason for hiding this comment

javanna May 3, 2018 • edited Loading

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

cbuescher left a comment

Choose a reason for hiding this comment

cbuescher commented May 3, 2018

javanna commented May 11, 2018

Van0SS commented May 12, 2018

cbuescher commented May 14, 2018

cbuescher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented May 14, 2018

Van0SS commented May 14, 2018

cbuescher commented May 15, 2018

cbuescher 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

cbuescher left a comment

Choose a reason for hiding this comment

cbuescher commented May 16, 2018

cbuescher commented May 16, 2018

Van0SS commented May 16, 2018

Van0SS Apr 18, 2018 •

edited

Loading

Van0SS Apr 20, 2018 •

edited

Loading

javanna May 3, 2018 •

edited

Loading

Van0SS Apr 18, 2018 •

edited

Loading

javanna May 3, 2018 •

edited

Loading