HLRC support for getTask #35166

markharwood · 2018-11-01T16:15:17Z

Given a GetTaskRequest the API returns an Optional which is empty in the case of 404s or returns a GetTaskResponse object if found.
Also added Helper methods in RestHighLevelClient for returning empty Optionals when hitting 404s as discussed here

Relates to #27205

elasticmachine · 2018-11-01T16:15:20Z

Pinging @elastic/es-core-infra

pgomulka · 2018-11-05T15:17:23Z

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

+        this.nodeId = nodeId;
+        this.taskId = taskId;
+    }
+


some redundant empty lines here, can we remove them?

pgomulka · 2018-11-05T15:20:12Z

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

+    public static GetTaskResponse fromXContent(XContentParser parser) {
+        return PARSER.apply(parser, null);
+    }    
+}


some minor new lines formatting would be nice to fix

pgomulka · 2018-11-05T15:23:46Z

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

+        request.setJsonEntity("{" + "  \"source\": {\n" + "    \"index\": \"source1\"\n" + "  },\n" + "  \"dest\": {\n"
+                + "    \"index\": \"dest\"\n" + "  }" + "}");
+        Response response = lowClient.performRequest(request);
+        String responseBody = EntityUtils.toString(response.getEntity());


feel free to use entityAsMap - available from super dlass

pgomulka · 2018-11-05T15:26:06Z

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

+        TaskId childTaskId = new TaskId(taskId.toString());
+        GetTaskRequest gtr = new GetTaskRequest(childTaskId.getNodeId(), childTaskId.getId());
+        Optional<GetTaskResponse> getTaskResponse = execute(gtr, highLevelClient().tasks()::get, highLevelClient().tasks()::getAsync);
+        assertTrue(getTaskResponse.isPresent());


this or assertions below might become flakey as there is no guarantee that the task will finish so quickly
would you consider waiting here for the status to finish?

BooleanSupplier hasUpgradeCompleted = checkCompletionStatus(task); awaitBusy(hasUpgradeCompleted); } } private BooleanSupplier checkCompletionStatus(TaskId taskId) { return () -> { try { Response response = client().performRequest(new Request("GET", "/_tasks/" + taskId.toString())); return (boolean) entityAsMap(response).get("completed"); } catch (IOException e) { fail(e.getMessage()); return false; } }; }

have a look at my draft https://github.com/elastic/elasticsearch/pull/35202/files

Is that a problem? My test is only asserting that the taskID exists and has the right description. The status (completed Vs in-progress) is not assumed and presumably it's beneficial for the test to potentially exercise both scenarios?

fair point, but is this guaranteed that Optional.isPresent will always pass? I think it is possible that with unlucky timing the task will not be created by the time we call getTaskResponse

as we discussed, that would be impossible to get an ID without a resource on already being created, loooks good!

pgomulka

This looks very good to me, but left some minor comments

hub-cap · 2018-11-05T16:55:27Z

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

+        try {
+            response = client.performRequest(req);
+        } catch (ResponseException e) {
+            if (404 == e.getResponse().getStatusLine().getStatusCode()) {


can you use RestStatus.NOT_FOUND.getStatus() here instead of just 404. The async method will need to be updated too.

hub-cap · 2018-11-05T17:04:19Z

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

+        client.performRequestAsync(req, responseListener);        
+    }    
+
+


hub-cap · 2018-11-05T17:04:31Z

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

@@ -1387,6 +1387,39 @@ public final void fieldCapsAsync(FieldCapabilitiesRequest fieldCapabilitiesReque
            throw new IOException("Unable to parse response body for " + response, e);
        }
    }
+
+


hub-cap · 2018-11-05T17:17:17Z

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

-                    assertThat("the return type for method [" + method + "] is incorrect",
-                        method.getReturnType().getSimpleName(), endsWith("Response"));
+                    // It's acceptable for 404s to be represented as empty Optionals 
+                    if (!method.getReturnType().isAssignableFrom(Optional.class)) {


can we ensure that the thing in the optional endsWith("Response"), as well?

I did have a stab at that and I'm not sure I can get at it via reflection. The best I could get was that the Optional class used in the response has a generic type of T. I suspect type erasure might prevent me from seeing that level of detail at runtime.

ok, i was unsure myself :)

hub-cap · 2018-11-05T17:43:48Z

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

+import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
+
+public class GetTaskResponse {
+    private final boolean completed;


spacing is messed up here and below the getters

hub-cap · 2018-11-05T18:12:11Z

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

+    public Optional<GetTaskResponse> get(GetTaskRequest request, RequestOptions options) throws IOException {
+        return restHighLevelClient.performRequestAndParseOptionalEntity(request, TasksRequestConverters::getTask, options,
+                GetTaskResponse::fromXContent);        
+


~~spaces~~ extra line

hub-cap · 2018-11-05T18:12:25Z

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

+        restHighLevelClient.performRequestAsyncAndParseOptionalEntity(request, TasksRequestConverters::getTask, options,
+                GetTaskResponse::fromXContent, listener);
+    }    
+


hub-cap · 2018-11-05T18:12:49Z

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

@@ -54,4 +55,12 @@ static Request listTasks(ListTasksRequest listTaskRequest) {
            .putParam("group_by", "none");
        return request;
    }
+
+    static Request getTask(GetTaskRequest getTaskRequest) {
+        Request request = new Request(HttpGet.METHOD_NAME, "_tasks/" + getTaskRequest.getNodeId() + ":" + getTaskRequest.getTaskId());


can you use the EndpointBuilder here?

Also added a method to EndpointBuilder for colon separated parts

hub-cap · 2018-11-05T18:13:21Z

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

+    static Request getTask(GetTaskRequest getTaskRequest) {
+        Request request = new Request(HttpGet.METHOD_NAME, "_tasks/" + getTaskRequest.getNodeId() + ":" + getTaskRequest.getTaskId());
+        RequestConverters.Params params = new RequestConverters.Params(request);
+        params.withTimeout(getTaskRequest.getTimeout()).withWaitForCompletion(getTaskRequest.getWaitForCompletion());


can u put these on 2 lines? u can keep builder if u want, its just easier to see it split on 2 lines

hub-cap

minor nits. i dont need to see it again.

hub-cap · 2018-11-06T19:18:05Z

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

+
+    static Request getTask(GetTaskRequest getTaskRequest) {
+        String endpoint = new EndpointBuilder().addPathPartAsIs("_tasks")
+                .addColonSeparatedPathParts(getTaskRequest.getNodeId(), Long.toString(getTaskRequest.getTaskId()))


I was thinking this couldve just been a

.addPathPart(getTaskRequest.getNodeId() + ":" + Long.toString(getTaskRequest.getTaskId()))

since its technically one "path part" in a URL. Then there is no need for an extra method there. I was just trying to ensure we were using the EndpointBuilder in all the RequestConverters.

OK. I saw a precedent for comma-delimited parts in EndPointBuilder and assumed colon-delimited would be another variant. I guess it's not that common a convention so I removed it.

hub-cap · 2018-11-06T19:20:56Z

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

+    }
+
+    /**
+     * Timeout to wait for any async actions this request must take. It must take anywhere from 0 to 2.


Im not sure i like these comments.. I know they are in server, but what does "it must take anywhere from 0 to 2" mean...?

Also, if there is a upper bound, maybe we should validate it in validate()

This is a copy-and-paste from the server equivalent. I'll take a closer look

Removed the comment. I don't see any validation code related to it so did not add anything to a validate() method. (General note: if we duplicate validation logic client and server-side is that not trappy code-maintenance-wise?)

hub-cap · 2018-11-06T19:22:47Z

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

@@ -1014,6 +1014,11 @@ EndpointBuilder addPathPart(String... parts) {
            return this;
        }

+        EndpointBuilder addColonSeparatedPathParts(String... parts) {


im not sure this is needed, we can handle it in the method.

hub-cap · 2018-11-06T19:25:47Z

after approving, I realized we did not have a GetTaskResponseTest, which would use the AbstractXContentTestCase.xContentTester. Id be happy to look at it again once you create the test, or you can just merge it.

Given a GetTaskRequest the API returns an Optional which is empty in the case of 404s or returns a TaskInfo object if found. Added Helper methods in RestHighLevelClient for returning empty Optionals when hitting 404s

… (this is acceptable for 404s)

…th new method), added (inconclusive) test for situation where GetTaskRequest has waitForCompletion=true. Conclusive test would require long-running tasks.

…vadoc

* master: Address handling of OS pretty name on some OS (elastic#35451) HLRC support for getTask (elastic#35166) upgrade to lucene-8.0.0-snapshot-6d9c714052 (elastic#35428) Add docs for CCR stats API (elastic#35439) Fix the names of CCR stats endpoints in usage API (elastic#35438) Switch to default to no build qualifier (elastic#35415) Clarify S3 repository storage class parameter (elastic#35400) SQL: Fix query translation for scripted queries (elastic#35408) [TEST] Instead of ignoring the ccr downgrade to basic license qa test avoid the assertions that check the log files, because that does not work on Windows. The rest of the test is still useful and should work on Windows CI. Upgrade to Joda 2.10.1 (elastic#35410) HLRest: model role and privileges (elastic#35128)

* master: Address handling of OS pretty name on some OS (elastic#35451) HLRC support for getTask (elastic#35166) upgrade to lucene-8.0.0-snapshot-6d9c714052 (elastic#35428)

* master: (22 commits) Introduce CCR getting started guide (elastic#35434) Correct typo in BuildPlugin exception message (elastic#35458) Correct implemented interface of ParsedReverseNested (elastic#35455) [ML] Fix find_file_structure NPE with should_trim_fields (elastic#35465) Handle OS pretty name on old OS without OS release (elastic#35453) Register remote cluster compress setting (elastic#35464) [DOCS] Clarify results_index_name description (elastic#35463) [TEST] add missing doc tests for HLRC GetRollupIndexCaps API [HLRC] Add GetRollupIndexCaps API (elastic#35102) Geo: enables coerce support in WKT polygon parser (elastic#35414) [ILM] fix retry so it picks up latest policy and executes async action (elastic#35406) Address handling of OS pretty name on some OS (elastic#35451) HLRC support for getTask (elastic#35166) upgrade to lucene-8.0.0-snapshot-6d9c714052 (elastic#35428) Add docs for CCR stats API (elastic#35439) Fix the names of CCR stats endpoints in usage API (elastic#35438) Switch to default to no build qualifier (elastic#35415) Clarify S3 repository storage class parameter (elastic#35400) SQL: Fix query translation for scripted queries (elastic#35408) [TEST] Instead of ignoring the ccr downgrade to basic license qa test avoid the assertions that check the log files, because that does not work on Windows. The rest of the test is still useful and should work on Windows CI. ...

markharwood added WIP v7.0.0 :Core/Java High Level REST Client labels Nov 1, 2018

markharwood force-pushed the fix/27205getTask branch 2 times, most recently from c5ec7ea to b3d2af0 Compare November 5, 2018 09:50

markharwood removed the WIP label Nov 5, 2018

markharwood requested a review from hub-cap November 5, 2018 13:47

pgomulka reviewed Nov 5, 2018

View reviewed changes

hub-cap suggested changes Nov 5, 2018

View reviewed changes

hub-cap mentioned this pull request Nov 5, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

hub-cap reviewed Nov 5, 2018

View reviewed changes

markharwood force-pushed the fix/27205getTask branch 2 times, most recently from d8665bf to 753677c Compare November 6, 2018 16:20

hub-cap approved these changes Nov 6, 2018

View reviewed changes

hub-cap mentioned this pull request Nov 6, 2018

HLRC: Add delete user action #35294

Merged

markharwood force-pushed the fix/27205getTask branch 2 times, most recently from fe38da3 to 3fc4226 Compare November 9, 2018 11:08

markharwood added 4 commits November 12, 2018 12:15

HLRC support for getTask.

3db51c3

Given a GetTaskRequest the API returns an Optional which is empty in the case of 404s or returns a TaskInfo object if found. Added Helper methods in RestHighLevelClient for returning empty Optionals when hitting 404s

Unused import

438dbe7

Relaxed RestHighLevelClientTests to allow methods to return Optionals…

7ae45d2

… (this is acceptable for 404s)

Remove unused imports

3b5aaf4

markharwood added 5 commits November 12, 2018 12:15

Javadoc fix for new method

dd4cb00

Fixed various whitespace violations, added use of EndpointBuilder (wi…

d43d008

…th new method), added (inconclusive) test for situation where GetTaskRequest has waitForCompletion=true. Conclusive test would require long-running tasks.

Unused imports

6ab1881

Added GetTaskResponseTest, tidied endpoint building, removed weird Ja…

4a1e344

…vadoc

Renamed due to failing name convention checks

ff8436f

markharwood force-pushed the fix/27205getTask branch from 3fc4226 to ff8436f Compare November 12, 2018 12:15

markharwood merged commit 64e5c25 into elastic:master Nov 12, 2018

markharwood added the backport pending label Nov 12, 2018

markharwood added v6.5.0 v6.6.0 and removed backport pending v6.5.0 labels Nov 13, 2018

tomcallahan added the >enhancement label Dec 18, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HLRC support for getTask #35166

HLRC support for getTask #35166

markharwood commented Nov 1, 2018 •

edited

Loading

elasticmachine commented Nov 1, 2018

pgomulka Nov 5, 2018

pgomulka Nov 5, 2018

pgomulka Nov 5, 2018

pgomulka Nov 5, 2018 •

edited

Loading

markharwood Nov 5, 2018

pgomulka Nov 6, 2018

pgomulka Nov 6, 2018

pgomulka left a comment

hub-cap Nov 5, 2018

hub-cap Nov 5, 2018

hub-cap Nov 5, 2018

hub-cap Nov 5, 2018

markharwood Nov 6, 2018

hub-cap Nov 6, 2018

hub-cap Nov 5, 2018

hub-cap Nov 5, 2018 •

edited

Loading

hub-cap Nov 5, 2018

hub-cap Nov 5, 2018

markharwood Nov 6, 2018

hub-cap Nov 5, 2018

hub-cap left a comment

hub-cap Nov 6, 2018

markharwood Nov 8, 2018

hub-cap Nov 6, 2018

hub-cap Nov 6, 2018

markharwood Nov 7, 2018

markharwood Nov 8, 2018

hub-cap Nov 6, 2018

hub-cap commented Nov 6, 2018

HLRC support for getTask #35166

HLRC support for getTask #35166

Conversation

markharwood commented Nov 1, 2018 • edited Loading

elasticmachine commented Nov 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgomulka Nov 5, 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

pgomulka 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

hub-cap Nov 5, 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

hub-cap 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

hub-cap commented Nov 6, 2018

markharwood commented Nov 1, 2018 •

edited

Loading

pgomulka Nov 5, 2018 •

edited

Loading

hub-cap Nov 5, 2018 •

edited

Loading