Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

high level REST api: cancel task #30745

Merged
merged 21 commits into from
Jun 7, 2018

Conversation

pcsanwald
Copy link
Contributor

The goal of this PR is to expose the cancel task functionality from the high level REST client, as part of #27205.

Still a few things to do on this, but wanted to put it up early in case I've gone off the rails with any of this. There are three things in particular I am unsure about:

1 - I'm unhappy with my integration testCase in ClusterClientIT, some options to make it better include using a similar technique to TasksIT.testTasksCancellation (which feels like duplication but an ok approach), or alternatively, passing something through that I can write better assertions against somehow. would love thoughts here.

2 - The response for cancelTask is the same as listTasks, for the documentation, should I copy paste and change the verb, or just say "the response is the same"?

3 - In terms of testing this locally against a running server, it seems like a valuable thing to do, but does need a task that will block. Is there a suitable task or do I need to do it via plugin somehow?

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra


CancelTasksRequest request = new CancelTasksRequest();
request.setTaskId(new TaskId(node, firstTask.getId()));
request.setReason("persistent action was removed");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably do something like "testreason" so no one gets confused.

highLevelClient().cluster()::cancelTasksAsync
);

assertThat(response, notNullValue());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd certainly be nice to have more here but you can't really be sure that you are going to actually cancel the task or it is going to finish. Since we already have an integration test for the act of cancelling such a task I'd probably be satisfied with something fairly basic here and good unit testing on the parsing side of the responses. We know the cancel works because we have TaskIT. Reindex even has some tests to make sure that it cancels itself in a sane way. So I think we're ok if we make sure we can parse here.

I do think it is worth leaving a comment about how this might cancel a task and it might not, depending on how quickly the task executes.

/**
* Returns the list of tasks that were cancelled
*/
public class CancelTasksResponse extends ListTasksResponse {

private static final ConstructingObjectParser<CancelTasksResponse, Void> PARSER =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think you can make something like:

protected static <T> ConstructingObjectParser<T, Void> setupParser(String name, TriFunction<List<TaskInfo>, List<TaskOperationFailure>, List<ElasticsearchException>> ctor)

in ListTasksResponse? That way you wouldn't need to copy this stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great idea! will do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nik9000 I went to do this and ran into a problem: setupParser would maybe need to instantiate a response: return new ListTasksResponse(tasks, tasksFailures, nodeFailures);, someplace, were you thinking that the calling method would supply a lambda to do this?

because otherwise, unless I'm missing something, couldn't call new on a generic type. Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd supply a lambda, yeah.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nik9000 nice suggestion, this is implemented now

@@ -50,9 +50,9 @@
* Returns the list of tasks currently running on the nodes
*/
public class ListTasksResponse extends BaseTasksResponse implements ToXContentObject {
private static final String TASKS = "tasks";
private static final String TASK_FAILURES = "task_failures";
private static final String NODE_FAILURES = "node_failures";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you made that method I mentioned about then these could stay private which is kind of nice.

@nik9000
Copy link
Member

nik9000 commented May 21, 2018

2 - The response for cancelTask is the same as listTasks, for the documentation, should I copy paste and change the verb, or just say "the response is the same"?

I'd say the response is the same and leave a link.

3 - In terms of testing this locally against a running server, it seems like a valuable thing to do, but does need a task that will block. Is there a suitable task or do I need to do it via plugin somehow?

Reindex can be made to take a long time if you index a bunch of documents. It is my go-to for manual testing for things like this.


==== Cancel Tasks Response

TBD
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably paste what we have in the other page, for simplicity.


@Override
protected boolean supportsUnknownFields() {
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be set to true so we test forward compatibility. Did you set it to false because you encountered problems with this?

Copy link
Contributor Author

@pcsanwald pcsanwald May 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javanna I did have problems with this value set to true, here's a gist with the error I saw.

I used the ListTask stuff as kind of a template for my changes, and I noticed we also set this value to false in ListTasksResponseTest.java, so I figured I'd need to do the same thing here. Is there a problem in both places? I'm not sure I fully understand the reason behind the failure, actually. I'll have a closer look at why this is happening, if the setting seems incorrect (there's a fair amount of me looking at other examples in the codebase for things on this PR)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the gist looks about the xcontent equivalence checks more than unknown fields. That is kind of expected whenever the response holds an exception, as we parse any exception back into a generic ElasticsearchException, which has a different toXContent output compared to the original exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a bit more debugging here: The problem, which also exists in ListTasksResponseTests, is that the headers field isn't being parsed correctly. I initially thought that this was because we were not overriding getRandomFieldsExcludeFilter (which TaskInfoTest does do) on either ListTasksResponseTests or CancelTasksResponseTests, but, I still am getting a parse error on the headers field.

The error seems to happen most often when the headers field is an empty map (which obv happens in a subset of the cases because it's random). Any thoughts on where the problem might be? I'm still not familiar enough with this code to triangulate, although I'm getting there :).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look and reworked a bit tests for list tasks here: #31001 . That should clarify what should and should not work in these tests, your cancel response tests should end up looking very similar. headers and status need to be both excluded, then we can enable the insertion of random fields by setting supportsUnknownFields to true. Also, xcontent equivalence works only when there are no failures, hence I split the two scenarios (with and without failures).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't really answer your question on the empty map problem that you saw, I don't quite follow on the empty map issue but I think it may have to do with the exclusion predicate. I have return field -> field.endsWith("status") || field.endsWith("headers");. Note the endsWith here compared to the equals in TaskInfoTests, that is because such fields are at the top-level for TaskInfo but deeper in the ListTasksResponse or CancelTasksResponse output.


@Override
protected boolean assertToXContentEquivalence() {
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is due to parsing exceptions back I guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another case of me doing the same thing as ListTasksResponseTest (perhaps incorrectly): here's a gist with the error when the value is set to true. I'm happy to dig deeper into this error if the setting seems incorrect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be ok!

* <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/tasks.html"> Task Management API on elastic.co</a>
* </p>
*/
public CancelTasksResponse cancelTasks(CancelTasksRequest cancelTasksRequest, Header... headers) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that these API should belong to the tasks namespace rather than cluster, according to our API spec, see #30906 .

@pcsanwald
Copy link
Contributor Author

@javanna @nik9000 I've still got a bit more work to do to address your comments, wanted to get namespace changes done first. will drop a note (maybe tomorrow) when this is ready for re-review.

This commit reworks testing for `ListTasksResponse` so that random
fields insertion can be tested and xcontent equivalence can be checked
too. Proper exclusions need to be configured, and failures need to be
tested separately. This helped finding a little problem, whenever there
is a node failure returned, the nodeId was lost as it was never printed
out as part of the exception toXContent.
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.notNullValue;

public class CancelTaskResponseTests extends AbstractXContentTestCase<CancelTasksResponse> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: would you mind calling it CancelTasksResponseTests to align it with the class that we are testing? and remove one of the whitespaces between the class name and extends? :)

public void deletePipelineAsync(DeletePipelineRequest request, ActionListener<WritePipelineResponse> listener, Header... headers) {
restHighLevelClient.performRequestAsyncAndParseEntity( request, RequestConverters::deletePipeline,
WritePipelineResponse::fromXContent, listener, emptySet(), headers);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These methods should be gone now, they have been moved to IngestClient upstream

* <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/tasks.html"> Task Management API on elastic.co</a>
* </p>
*/
public void cancelTasksAsync(CancelTasksRequest cancelTasksRequest, ActionListener<CancelTasksResponse> listener, Header... headers) {
Copy link
Member

@javanna javanna Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that #30490 is in, could you replace the header argument with the RequestOptions one? In the method that accept a listener we add RequestOptions between the request and the listener though.

WritePipelineResponse response =
execute(request, highLevelClient().cluster()::deletePipeline, highLevelClient().cluster()::deletePipelineAsync);
assertTrue(response.isAcknowledged());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the changes should not be here, these tests have been moved to IngestClientIT upstream

cancelTasksRequest.getTimeout())
.withTaskId(cancelTasksRequest.getTaskId())
.withNodes(cancelTasksRequest.getNodes())
.withActions(cancelTasksRequest.getActions());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the REST action seems to also support parent_task_id, the parameter is also declared in the REST spec so it should be here too?

* <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/tasks.html"> Task Management API on elastic.co</a>
* </p>
*/
public CancelTasksResponse cancelTasks(CancelTasksRequest cancelTasksRequest, Header... headers) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we call the methods just cancel given that they now belong to the TasksClient?

CancelTasksRequest request = new CancelTasksRequest();
Map<String, String> expectedParams = new HashMap<>();
TaskId taskId = new TaskId(randomAlphaOfLength(5), randomNonNegativeLong());
request.setTaskId(taskId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we also test the other supported parameters?


@Override
protected boolean assertToXContentEquivalence() {
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in theory this could be left as default (true) given that failures are tested separately?

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some more comments but this is very close.

@@ -70,6 +70,17 @@ protected static RestHighLevelClient highLevelClient() {
}
}

protected static <Req, Resp> Resp executeWithOptions(Req request, SyncMethodWithRequestOptions<Req, Resp> syncMethod,
AsyncMethodWithRequestOptions<Req, Resp> asyncMethod, RequestOptions options) throws IOException {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after some discussion with @nik9000, I tried refactoring the existing execute and associated functional interfaces, and it seems like a big enough refactor that I would rather not include in this PR. I'm happy to follow up on a separate PR, but these changes make it possible to migrate the *IT.java tests piecemeal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha, this has to do with #31069 then, which I will get in today hopefully, then no refactorings should be needed.

@pcsanwald pcsanwald force-pushed the rest-high-level-cancel-task branch from 36e9fcf to 0c05fe7 Compare June 6, 2018 22:35
@pcsanwald
Copy link
Contributor Author

@javanna @nik9000 I've merged latest changes from master; this should be ready for final review!

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some super minor nits, LGTM. Feel free to merge once those are addressed, all good on my end. Thanks!

Request request = new Request(HttpPost.METHOD_NAME, "/_tasks/_cancel");
Params params = new Params(request);
params.withTimeout(
cancelTasksRequest.getTimeout())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can you adjust the indentation?

@@ -95,4 +95,5 @@ public void putSettingsAsync(ClusterUpdateSettingsRequest clusterUpdateSettingsR
restHighLevelClient.performRequestAsyncAndParseEntity(clusterUpdateSettingsRequest, RequestConverters::clusterPutSettings,
ClusterUpdateSettingsResponse::fromXContent, listener, emptySet(), headers);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove empty line?

@@ -65,4 +67,45 @@ public void listAsync(ListTasksRequest request, RequestOptions options, ActionLi
restHighLevelClient.performRequestAsyncAndParseEntity(request, RequestConverters::listTasks, options,
ListTasksResponse::fromXContent, listener, emptySet());
}

/**
* Cancel one or more cluster tasks using the Task Management API
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a punctuation mark at the end of the sentence? It will make the generated html slightly better

}

/**
* Asynchronously cancel one or more cluster tasks using the Task Management API
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, can you add the punctuation mark

@@ -108,4 +108,5 @@ public void testClusterUpdateSettingNonExistent() {
assertThat(exception.getMessage(), equalTo(
"Elasticsearch exception [type=illegal_argument_exception, reason=transient setting [" + setting + "], not recognized]"));
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove empty line?

highLevelClient().tasks()::listAsync
);

// TODO[PCS] submit a task that is cancellable and assert it's cancelled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this TODO still valid? do we need to address it before merging?

cancelTasksRequest.setReason("testreason");

CancelTasksResponse response = execute(cancelTasksRequest, highLevelClient().tasks()::cancel, highLevelClient().tasks()::cancelAsync
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: adjust indentation?

@@ -178,4 +178,5 @@ public void onFailure(Exception e) {
assertTrue(latch.await(30L, TimeUnit.SECONDS));
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove empty line?


@Override
public String toString() {
return Strings.toString(this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we pretty print the output?

return true;
}


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove one of the two empty lines?

@pcsanwald pcsanwald merged commit e82e5cc into elastic:master Jun 7, 2018
@pcsanwald pcsanwald deleted the rest-high-level-cancel-task branch June 7, 2018 21:02
jasontedor added a commit that referenced this pull request Jun 8, 2018
* elastic/master: (53 commits)
  Painless: Restructure/Clean Up of Spec Documentation (#31013)
  Update ignore_unmapped serialization after backport
  Add back dropped substitution on merge
  high level REST api: cancel task (#30745)
  Enable engine factory to be pluggable (#31183)
  Remove vestiges of animal sniffer (#31178)
  Rename elasticsearch-nio to nio (#31186)
  Rename elasticsearch-core to core (#31185)
  Move cli sub-project out of server to libs (#31184)
  [DOCS] Fixes broken link in auditing settings
  QA: Better seed nodes for rolling restart
  [DOCS] Moves ML content to stack-docs
  [DOCS] Clarifies recommendation for audit index output type (#31146)
  Add nio-transport as option for http smoke tests (#31162)
  QA: Set better node names on rolling restart tests
  Add support for ignore_unmapped to geo sort (#31153)
  Share common parser in some AcknowledgedResponses (#31169)
  Fix random failure on SearchQueryIT#testTermExpansionExceptionOnSpanFailure
  Remove reference to multiple fields with one name (#31127)
  Remove BlobContainer.move() method (#31100)
  ...
@javanna
Copy link
Member

javanna commented Jun 8, 2018

@pcsanwald are you going to backport this to 6.x?

@pcsanwald
Copy link
Contributor Author

pcsanwald commented Jun 8, 2018 via email

pcsanwald pushed a commit that referenced this pull request Jun 8, 2018
* Initial commit of rest high level exposure of cancel task

* fix javadocs

* address some code review comments

* update branch to use tasks namespace instead of cluster

* High-level client: list tasks failure to not lose nodeId

This commit reworks testing for `ListTasksResponse` so that random
fields insertion can be tested and xcontent equivalence can be checked
too. Proper exclusions need to be configured, and failures need to be
tested separately. This helped finding a little problem, whenever there
is a node failure returned, the nodeId was lost as it was never printed
out as part of the exception toXContent.

* added comment

* merge from master

* re-work CancelTasksResponseTests to separate XContent failure cases from non-failure cases

* remove duplication of logic in parser creation

* code review changes

* refactor TasksClient to support RequestOptions

* add tests for parent task id

* address final PR review comments, mostly formatting and such
@pcsanwald
Copy link
Contributor Author

pcsanwald commented Jun 8, 2018

@javanna backported into 6.x with 540b117

@javanna
Copy link
Member

javanna commented Jun 8, 2018

nice thanks @pcsanwald !

dnhatn added a commit that referenced this pull request Jun 10, 2018
* 6.x:
  Move default location of dependencies report (#31228)
  Remove dependencies report task dependencies (#31227)
  Add recognition of MPL 2.0 (#31226)
  Fix unknown licenses (#31223)
  Fully encapsulate LocalCheckpointTracker inside of the engine (#31213)
  Remove version from license file name for GCS SDK (#31221)
  Remove DocumentFieldMappers#simpleMatchToFullName. (#31041)
  [DOCS] Removes 6.3.1 release notes
  [DOCS] Splits release notes by major version
  Remove DocumentFieldMappers#smartNameFieldMapper, as it is no longer needed. (#31018)
  Remove extraneous references to 'tokenized' in the mapper code. (#31010)
  SQL: Make a single JDBC driver jar (#31012)
  QA: Fix rolling restart tests some more
  Allow to trim all ops above a certain seq# with a term lower than X
  high level REST api: cancel task (#30745)
  Mute TokenBackwardsCompatibilityIT.testMixedCluster
  Mute WatchBackwardsCompatibilityIT.testWatcherRestart
  Enhance license detection for various licenses (#31198)
  [DOCS] Add note about long-lived idle connections (#30990)
  Add high-level client methods that accept RequestOptions (#31069)
  Remove RestGetAllMappingsAction (#31129)
  Move RestGetSettingsAction to RestToXContentListener (#31101)
  Move number of language analyzers to analysis-common module (#31143)
  flush job to ensure all results have been written (#31187)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants