Add Snapshots Status API to High Level Rest Client #31515

jbaiera · 2018-06-21T19:23:52Z

This PR adds the Snapshots Status API to the High Level Rest Client. There are
a few points that are strange, mainly that some of the response objects serialize
themselves differently across XContent and regular Writables.

Relates #27205

SnapshotStats fromXContent added. SnapshotSharsStats fromXContent added. SnapshotIndexShardStatus fromXContent added. SnapshotIndexStatus fromXContent added. SnapshotStatus fromXContent added. SnapshotStatusResponse fromXContent added. Add RequestConverter code. Add snapshots status apis to the snapshot client. Add testing and documentation for snapshots status api

elasticmachine · 2018-06-21T19:23:54Z

Pinging @elastic/es-core-infra

hub-cap

Id love to do another pass once you convert those older parsed fromXContent methods to use an ObjectParser :)

hub-cap · 2018-06-25T14:26:55Z

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

+
+        Params parameters = new Params(request);
+        parameters.withMasterTimeout(snapshotsStatusRequest.masterNodeTimeout());
+        parameters.putParam("ignore_unavailable", Boolean.toString(snapshotsStatusRequest.ignoreUnavailable()));


it might make sense to build out ignore_unavailable into a method now that its used in 2x places.

hub-cap · 2018-06-25T14:29:12Z

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

+        createSnapshot.addParameter("wait_for_completion", "true");
+        createSnapshot.setEntity(new NStringEntity("{\"indices\":\""+index+"\"}", ContentType.APPLICATION_JSON));
+        return highLevelClient().getLowLevelClient().performRequest(createSnapshot);
+    }


You should rebase on 90d62e6. It included a createTestSnapshot method here as well for his tests.

hub-cap · 2018-06-25T14:30:47Z

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

+        PutRepositoryResponse putRepositoryResponse = createTestRepository("test", FsRepository.TYPE, "{\"location\": \".\"}");
+        assertTrue(putRepositoryResponse.isAcknowledged());
+
+        createIndex("test_index", Settings.EMPTY);


maybe variablize test_index so its reused in the next few places

hub-cap · 2018-06-25T14:32:46Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotIndexShardStatus.java

+        String nodeId = null;
+        String failure = null;
+        SnapshotStats stats = null;
+        while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {


any reason that you decided to not use an ObjectParser here?

This comment applies to all of the fromXContent methods.

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

...evel/src/test/java/org/elasticsearch/client/documentation/SnapshotClientDocumentationIT.java

+        // end::snapshots-status-request-snapshots
+        // tag::snapshots-status-request-ignoreUnavailable
+        request.ignoreUnavailable(true); // <1>
+        // end::snapshots-status-request-ignoreUnavailable


These are not actually rendered since the doc markup does not have them included in it.

LOL disregard clicked the wrong link!

hub-cap · 2018-06-25T14:49:48Z

Verified the doc build works.

javanna

left a couple of comments too

javanna · 2018-06-26T13:35:57Z

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

+        Request request = RequestConverters.snapshotsStatus(snapshotsStatusRequest);
+        assertThat(request.getEndpoint(), equalTo(endpoint));
+        assertThat(request.getMethod(), equalTo(HttpGet.METHOD_NAME));
+        assertThat(request.getParameters(), equalTo(expectedParams));


test that the body is null?

Addressed in 1a85d5b

javanna · 2018-06-26T13:39:23Z

...c/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotShardsStats.java

+                        throw new ElasticsearchParseException("failed to parse snapshot shards stats, expected number for field [{}]",
+                            currentName);
+                    }
+                    totalShards = parser.intValue();


I wonder if it's necessary to throw specific errors for each case here. It is nice to have good error messages, on the other hand these would be thrown anyways (I think) by the parser, maybe a bit more cryptic. But most importantly, is it worth it given that we are parsing our own output. Certainly we are not going to change the type of these fields one way?

javanna · 2018-06-26T13:40:11Z

...c/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotShardsStats.java

+                    throw new ElasticsearchParseException("failed to parse snapshot shards stats, unexpected field [{}]", currentName);
+                }
+            } else {
+                throw new ElasticsearchParseException("failed to parse snapshot shards stats");


this is making the parser strict and I think it breaks forward compatibility. What if one day we add a new array or object at this level? This is also valid for the other parsers added with this PR. Converting them to object parser and providing true as supportsUnknownFields argument is also a valid fix.

javanna · 2018-06-26T13:44:44Z

...er/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotStatus.java

+            throw new ElasticsearchParseException("failed to parse snapshot status, missing snapshot repository");
+        } else if (state == null) {
+            throw new ElasticsearchParseException("failed to parse snapshot status, missing snapshot state");
+        }


I don't know if we need this validation, given that we parse our own output and we know that info is going to be there

javanna · 2018-06-26T13:46:40Z

...va/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotsStatusResponseTests.java

+
+    @Override
+    protected boolean supportsUnknownFields() {
+        return false;


all these should be true, which tests forward compatibility. they can't be true now because the new parsers are too strict. It may be that there are specific places where injecting random fields is really not possible, such specific locations can be excluded by overriding getRandomFieldsExcludeFilter

hub-cap

Im happy with this now, but I would like @javanna to also review since he had some concerns about compat with the parsing. so +0 from me.

Self dismissing

javanna

left a couple of comments and questions, nothing major, thanks @jbaiera !

javanna · 2018-06-29T11:15:12Z

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

+     * @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
+     * @param listener the listener to be notified upon request completion
+     */
+    public void snapshotsStatusAsync(SnapshotsStatusRequest snapshotsStatusRequest, RequestOptions options,


can you rename the methods to status ? That's how it's defined in our REST spec.

javanna · 2018-06-29T11:17:14Z

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

        Request createSnapshot = new Request("put", String.format(Locale.ROOT, "_snapshot/%s/%s", repository, snapshot));
        createSnapshot.addParameter("wait_for_completion", "true");
+        createSnapshot.setEntity(new NStringEntity("{\"indices\":\""+index+"\"}", ContentType.APPLICATION_JSON));


you can also use setJsonEntity if you wish, that's handy

javanna · 2018-06-29T11:18:54Z

...evel/src/test/java/org/elasticsearch/client/documentation/SnapshotClientDocumentationIT.java

+        // end::snapshots-status-execute
+
+        // tag::snapshots-status-response
+        List<SnapshotStatus> snapshotStatusesResponse = response.getSnapshots();


could you go a bit deeper here and show users what they can retrieve from the response? We don't need to show everything but I would add what we'd expect users to generally do.

javanna · 2018-06-29T11:25:54Z

docs/java-rest/high-level/snapshot/snapshots_status.asciidoc

+--------------------------------------------------
+include-tagged::{doc-tests}/SnapshotClientDocumentationIT.java[snapshots-status-request-snapshots]
+--------------------------------------------------
+<1> The list of snapshot names to check the status of. If this is set, the status for the snapshots


repository and snapshots are not optional arguments, we will throw exception if they are not provided (see validate method)

Hmm, this doesn't seem to match up with the current API documentation for this end point. If they are left off the API reports on any currently running snapshots. Not sure if that's a problem or a conscious decision to not support that aspect of the request.

seems to be a conscious decision given that we expose the /_snapshot/_status endpoint alone, which is also documented in our REST spec.

I pushed d9d35a6 for the required/optional arguments.

javanna · 2018-06-29T11:32:10Z

...er/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotStatus.java

+            try {
+                state = SnapshotsInProgress.State.valueOf(rawState);
+            } catch (IllegalArgumentException iae) {
+                throw new ElasticsearchParseException("failed to parse snapshot status, unknown state value [{}]", iae, rawState);


is this catch necessary?

javanna · 2018-06-29T11:32:31Z

...a/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotIndexShardStatusTests.java

+
+    @Override
+    protected Predicate<String> getRandomFieldsExcludeFilter() {
+        // Do not place random fields in the root object since its fields correspond to shard names.


++ thanks for the comment!!

javanna · 2018-06-29T11:33:25Z

...a/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotIndexShardStatusTests.java

+        XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation);
+        XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, parser.nextToken(), parser::getTokenLocation);
+        SnapshotIndexShardStatus status = SnapshotIndexShardStatus.fromXContent(parser, parser.currentName());
+        XContentParserUtils.ensureExpectedToken(XContentParser.Token.END_OBJECT, parser.nextToken(), parser::getTokenLocation);


this is a bit weird, why does the test need to consume some tokens, rather than the fromXContent itself?

The big issue here is that the stats objects write their own names out in their toXContent calls instead of just the object contents. This means that the test code automatically wraps them one layer deeper in an object, which needs to be unpeeled before testing the parsing. My original thought was to avoid changing that much for this PR, but it'll probably be far better to just fix the stats objects write logic.

I was expecting this and some others below to go away with the stats changes that you made. Is this logic still required?

I double checked, this is fine in the places where we have it now.

javanna · 2018-06-29T11:34:26Z

...t/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotIndexStatusTests.java

+        XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation);
+        XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, parser.nextToken(), parser::getTokenLocation);
+        SnapshotIndexStatus status = SnapshotIndexStatus.fromXContent(parser);
+        XContentParserUtils.ensureExpectedToken(XContentParser.Token.END_OBJECT, parser.nextToken(), parser::getTokenLocation);


same comment as above about consuming tokens in the test

As mentioned above, many of these objects are XContentFragments instead of objects. They are automatically wrapped in an object, and need to be unwrapped during the test parsing.

javanna · 2018-06-29T11:35:12Z

...t/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotShardsStatsTests.java

+        if (stats == null) {
+            throw new ElasticsearchParseException("could not find stats");
+        }
+        return stats;


here as well, why do we need this parsing logic in the test? Maybe it has to do with toXContent and fromXContent not matching 100% ?

javanna · 2018-06-29T11:40:21Z

...rc/test/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotStatsTests.java

+        if (stats == null) {
+            throw new ElasticsearchParseException("could not find stats");
+        }
+        return stats;


same comment as above on the parsing logic here.

Convert the stats objects to objects instead of fragments. Simplify unit tests. Add more response uses to the documentation Rename api method to 'status'

javanna

LGTM besides the docs argument section that needs updating. Thanks @jbaiera !

jbaiera · 2018-07-10T17:29:15Z

I pushed d9d35a6 for the docs argument section. Thanks all! ❤️

hub-cap · 2018-07-11T18:58:34Z

@jbaiera dont forget to check the checkbox on the meta issue once you are backported

* master: [TEST] Mute SlackMessageTests.testTemplateRender Docs: Explain closing the high level client [ML] Re-enable memory limit integration tests (#31328) [test] disable packaging tests for suse boxes Add nio transport to security plugin (#31942) XContentTests : Insert random fields at random positions (#30867) Force execution of fetch tasks (#31974) Fix unreachable error condition in AmazonS3Fixture (#32005) Tests: Fix SearchFieldsIT.testDocValueFields (#31995) Add Expected Reciprocal Rank metric (#31891) [ML] Get ForecastRequestStats doc in RestoreModelSnapshotIT (#31973) SQL: Add support for single parameter text manipulating functions (#31874) [ML] Ensure immutability of MlMetadata (#31957) Tests: Mute SearchFieldsIT.testDocValueFields() muted tests due to #31940 Work around reported problem in eclipse (#31960) Move build integration tests out of :buildSrc project (#31961) Tests: Remove use of joda time in some tests (#31922) [Test] Reactive 3rd party tests on CI (#31919) SQL: Support for escape sequences (#31884) SQL: HAVING clause should accept only aggregates (#31872) Docs: fix typo in datehistogram (#31972) Switch url repository rest tests to new style requests (#31944) Switch reindex tests to new style requests (#31941) Docs: Added note about cloud service to installation and getting started [DOCS] Removes alternative docker pull example (#31934) Add Snapshots Status API to High Level Rest Client (#31515) ingest: date_index_name processor template resolution (#31841) Test: fix null failure in watcher test (#31968) Switch test framework to new style requests (#31939) Switch low level rest tests to new style Requests (#31938) Switch high level rest tests to new style requests (#31937) [ML] Mute test failing due to Java 11 date time format parsing bug (#31899) [TEST] Mute SlackMessageTests.testTemplateRender Fix assertIngestDocument wrongfully passing (#31913) Remove unused reference to filePermissionsCache (#31923) rolling upgrade should use a replica to prevent relocations while running a scroll HLREST: Bundle the x-pack protocol project (#31904) Increase logging level for testStressMaybeFlush Added lenient flag for synonym token filter (#31484) [X-Pack] Beats centralized management: security role + licensing (#30520) HLRest: Move xPackInfo() to xPack().info() (#31905) Docs: add security delete role to api call table (#31907) [test] port archive distribution packaging tests (#31314) Watcher: Slack message empty text (#31596) [ML] Mute failing DetectionRulesIT.testCondition() test Fix broken NaN check in MovingFunctions#stdDev() (#31888) Date: Add DateFormatters class that uses java.time (#31856) [ML] Switch native QA tests to a 3 node cluster (#31757) Change trappy float comparison (#31889) Fix building AD URL from domain name (#31849) Add opaque_id to audit logging (#31878) re-enable backcompat tests add support for is_write_index in put-alias body parsing (#31674) Improve release notes script (#31833) [DOCS] Fix broken link in painless example Handle missing values in painless (#30975) Remove the ability to index or query context suggestions without context (#31007) Ingest: Enable Templated Fieldnames in Rename (#31690) [Docs] Fix typo in the Rollup API Quick Reference (#31855) Ingest: Add ignore_missing option to RemoveProc (#31693) Add template config for Beat state to X-Pack Monitoring (#31809) Watcher: Add ssl.trust email account setting (#31684) Remove link to oss-MSI (#31844) Painless: Restructure Definition/Whitelist (#31879) HLREST: Add x-pack-info API (#31870)

javanna · 2018-07-22T14:09:54Z

heads up @jbaiera when I backported #31825 I had to mark snapshot.status as not supported in RestHighLevelClientTests as this was not backported yet. You will have to remove the API from the list when backporting it then.

This PR adds the Snapshots Status API to the Snapshot Client, as well as additional documentation for the status api.

jbaiera · 2018-07-23T19:09:49Z

Thanks @javanna - I've opened a backport PR #32295

jbaiera added 3 commits June 21, 2018 12:32

Make checkstyle and forbidden apis happy

0fad8d0

Remove todos

2f05454

jbaiera added >enhancement :Core/Java High Level REST Client labels Jun 21, 2018

javanna mentioned this pull request Jun 21, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

jbaiera requested review from javanna and hub-cap June 22, 2018 15:35

hub-cap previously requested changes Jun 25, 2018

View reviewed changes

hub-cap reviewed Jun 25, 2018

View reviewed changes

jbaiera added 2 commits June 25, 2018 11:47

Merge branch 'master' into rest-snapshot-status

7725747

Address reviewer comments.

c2a591d

javanna requested changes Jun 26, 2018

View reviewed changes

jbaiera added 7 commits June 26, 2018 13:15

Convert most of the parsing logic over to object parsers.

104b723

Fix test, address reviewer comments.

1a85d5b

Change parsers to be lenient on unknown fields.

a58e592

Fix a line length issue that IDEA hid from me...

b89f8ca

Unused import

f061599

Merge branch 'master' into rest-snapshot-status

83c3ea2

Always explicitly set ignore_unavailable.

9024aad

hub-cap reviewed Jun 27, 2018

View reviewed changes

javanna reviewed Jun 29, 2018

View reviewed changes

jbaiera added 5 commits June 29, 2018 13:27

Address reviewer comments.

d9d35a6

Convert the stats objects to objects instead of fragments. Simplify unit tests. Add more response uses to the documentation Rename api method to 'status'

Make checkstyle happy

36ae856

Merge branch 'master' into rest-snapshot-status

4231ece

Fix test failures

1615f0a

Fix test error

8d5c999

javanna approved these changes Jul 9, 2018

View reviewed changes

jbaiera merged commit 5bcdff7 into elastic:master Jul 11, 2018

jbaiera deleted the rest-snapshot-status branch July 11, 2018 16:07

jbaiera added backport pending v7.0.0 v6.4.0 labels Jul 11, 2018

jbaiera added a commit to jbaiera/elasticsearch that referenced this pull request Jul 23, 2018

Add Snapshots Status API to High Level Rest Client (elastic#31515)

bdc0434

This PR adds the Snapshots Status API to the Snapshot Client, as well as additional documentation for the status api.

jbaiera mentioned this pull request Jul 23, 2018

Backport to 6.x - Add Snapshots Status API to High Level Rest Client #32295

Merged

jakelandis removed the backport pending label Jan 31, 2019

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

Add Snapshots Status API to High Level Rest Client #31515

Add Snapshots Status API to High Level Rest Client #31515

Conversation

jbaiera commented Jun 21, 2018

elasticmachine commented Jun 21, 2018

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 Jun 25, 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

hub-cap left a comment

Choose a reason for hiding this comment

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

javanna left a comment

Choose a reason for hiding this comment

jbaiera commented Jul 10, 2018

hub-cap commented Jul 11, 2018

javanna commented Jul 22, 2018

jbaiera commented Jul 23, 2018