Add GET Repository High Level REST API #30362

hub-cap · 2018-05-03T12:57:19Z

This commit adds the Modules Client with a first API call within it, the
get repositories call in snapshot/restore module. The test itself is
somewhat lacking, but that is due to the fact that there is no way to
insert more repositories until a PUT call is fleshed out.

Relates #27205

This commit adds the Modules Client with a first API call within it, the get repositories call in snapshot/restore module. The test itself is somewhat lacking, but that is due to the fact that there is no way to insert more repositories until a PUT call is fleshed out. Relates elastic#27205

elasticmachine · 2018-05-03T12:57:21Z

Pinging @elastic/es-core-infra

javanna

I left a couple of comments, @hub-cap do you plan on adding docs later once all repositories API are done? I don't mind as long as we make sure that we don't release these without docs (it happened before unfortunately, that's why I worry)

javanna · 2018-05-03T18:28:14Z

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

+ * <p>
+ * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/modules.html">Modules APIs on elastic.co</a>
+ */
+public final class ModulesClient {


why do we need a separate client for modules? I thought that we need something like this for plugins, but users may not know that some of our stuff is isolate in modules, given that it's all shipped with the rest of the codebase. I would add these methods to the existing client, yet I may have missed previous discussions around this, not sure.

see my overall comment.

javanna · 2018-05-03T18:29:32Z

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

+     *
+     * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/modules.html">Modules APIs on elastic.co</a>
+     */
+    public final ModulesClient modules() {


On the same line as my other comment above, I assume that this method is weird for users. Also, none of the other language clients have this namespace while they all have indices, cluster etc.

Totally wasnt aware, makes sense.

javanna · 2018-05-03T18:30:31Z

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

@@ -630,6 +631,17 @@ static Request indexPutSettings(UpdateSettingsRequest updateSettingsRequest) thr
        return request;
    }

+    static Request modulesGetRepositories(GetRepositoriesRequest getRepositoriesRequest) {


could we add a test for this method?

ill add, for sure 👍

javanna · 2018-05-03T18:31:17Z

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

+        GetRepositoriesRequest request = new GetRepositoriesRequest();
+        GetRepositoriesResponse response = execute(request, highLevelClient().modules()::getRepositories,
+            highLevelClient().modules()::getRepositoriesAsync);
+        // Once create repository is fleshed out, we will be able to do more with this test case.


couldn't we use the low-level REST client for this call? Maybe not worth it given that create is coming soon?

create is the next thing im working on :)

javanna · 2018-05-03T18:32:55Z

...in/java/org/elasticsearch/action/admin/cluster/repositories/get/GetRepositoriesResponse.java

@@ -74,4 +76,11 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
        builder.endObject();
        return builder;
    }
+
+    public static GetRepositoriesResponse fromXContent(XContentParser parser) throws IOException {
+        if (parser.nextToken() != XContentParser.Token.START_OBJECT) {


we could use ensureExpectedToken here? I even wonder if this check is needed, I don't think we do this kind of check consistently in our parsing code. Out of curiosity, what happens without it?

its possible this is a bug. I believe its because the toXContent above is calling builder.startObject, I dont know how we typically handle these. the code in RepositoriesMetaData checks for the tokens like this, so i went with that. Ill gladly change it given a better example.

leave it, it certainly doesn't hurt. maybe use ensureExceptedToken though

hub-cap · 2018-05-03T20:27:53Z

Hey @javanna as this is my first API call, i wasn't aware of the docs. I will be sure to look it up and add them as I go, lest I forget them as well!

As for separating them out, I only did this because the separation I thought we were doing (there is a different docs section for clusters and indices, so i figured this should follow suit). Ill gladly add them to the client itself, rather than in a special client. Let me try again :)

hub-cap · 2018-05-07T18:35:02Z

I am putting this on hold until I can find out what some of the clients do WRT the snapshot namespace. I would like to be consistent as possible. This includes all doc work (which is not yet started, but will be part of this PR). I have addressed the other PR comments.

hub-cap · 2018-05-07T19:49:33Z

After chatting with some of the lang clients, I found out that they tend to use the namespace defined in the rest-api-spec. In this case its snapshot.*, so I went ahead and made a SnapshotClient. I still need to do docs and a changelog entry :)

hub-cap · 2018-05-08T12:57:13Z

@javanna I have addressed the docs changes, as well as the PR requests. I have also run the docs build to make sure the high level rest client docs did not fail :)

hub-cap · 2018-05-08T13:11:22Z

@elasticmachine test it

hub-cap · 2018-05-08T13:11:43Z

@elasticmachine test this please

nik9000

It looks fine to me.

javanna

left a few small comments, LGTM otherwise, feel free to merge once you addressed those. Thanks!

javanna · 2018-05-08T13:53:32Z

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

@@ -792,6 +803,13 @@ Params withRefreshPolicy(WriteRequest.RefreshPolicy refreshPolicy) {
            return this;
        }

+        Params withRepository(String[] repositories) {
+            if (repositories != null && repositories.length > 0) {
+                return putParam("repository", Strings.arrayToCommaDelimitedString(repositories));


nit: shall we rather call the _snapshot/{repo} endpoint whenever one or more repo names are provided instead of using the query_string param?

yes, ive fixed this. Good call.

javanna · 2018-05-08T21:28:08Z

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

+        return snapshotClient;
+    }
+
+


super important: one less empty line? :)

javanna · 2018-05-08T21:28:25Z

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

+        this.restHighLevelClient = restHighLevelClient;
+    }
+
+


here too one less empty line?

javanna · 2018-05-08T21:28:47Z

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

+     * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-snapshots.html"> Snapshot and Restore
+     * API on elastic.co</a>
+     */
+


remove this empty line?

javanna · 2018-05-08T21:29:49Z

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

@@ -1427,6 +1428,24 @@ public void testIndexPutSettings() throws IOException {
        assertEquals(expectedParams, request.getParameters());
    }

+    public void testGetRepositories() {
+        Map<String, String> expectedParams = new HashMap<>();
+        expectedParams.put("master_timeout", "30s");


you can use setRandomTimeout. also can we test the local flag (you can use setRandomLocal I think)

thats awesome, i fixed the master timeout and local params by using these helpers. tyvm for showing them to me.

hub-cap · 2018-05-09T02:42:28Z

@elasticmachine test this please

…or-you * elastic/master: (22 commits) Docs: Test examples that recreate lang analyzers (elastic#29535) BulkProcessor to retry based on status code (elastic#29329) Add GET Repository High Level REST API (elastic#30362) add a comment explaining the need for RetryOnReplicaException on missing mappings Add `coordinating_only` node selector (elastic#30313) Stop forking groovyc (elastic#30471) Avoid setting connection request timeout (elastic#30384) Use date format in `date_range` mapping before fallback to default (elastic#29310) Watcher: Increase HttpClient parallel sent requests (elastic#30130) Mute ML upgrade test (elastic#30458) Stop forking javac (elastic#30462) Client: Deprecate many argument performRequest (elastic#30315) Docs: Use task_id in examples of tasks (elastic#30436) Security: Rename IndexLifecycleManager to SecurityIndexManager (elastic#30442) [Docs] Fix typo in cardinality-aggregation.asciidoc (elastic#30434) Avoid NPE in `more_like_this` when field has zero tokens (elastic#30365) Build: Switch to building javadoc with html5 (elastic#30440) Add a quick tour of the project to CONTRIBUTING (elastic#30187) Reindex: Use request flavored methods (elastic#30317) Silence SplitIndexIT.testSplitIndexPrimaryTerm test failure. (elastic#30432) ...

This commit adds the Snapshot Client with a first API call within it, the get repositories call in snapshot/restore module. This also creates a snapshot namespace for the docs, as well as get repositories docs. Relates #27205

* master: Upgrade to Lucene-7.4-snapshot-6705632810 (#30519) add version compatibility from 6.4.0 after backport, see #30319 (#30390) Security: Simplify security index listeners (#30466) Add proper longitude validation in geo_polygon_query (#30497) Remove Discovery.AckListener.onTimeout() (#30514) Build: move generated-resources to build (#30366) Reindex: Fold "with all deps" project into reindex (#30154) Isolate REST client single host tests (#30504) Solve Gradle deprecation warnings around shadowJar (#30483) SAML: Process only signed data (#30420) Remove BWC repository test (#30500) Build: Remove xpack specific run task (#30487) AwaitsFix IntegTestZipClientYamlTestSuiteIT#indices.split tests LLClient: Add setJsonEntity (#30447) Expose CommonStatsFlags directly in IndicesStatsRequest. (#30163) Silence IndexUpgradeIT test failures. (#30430) Bump Gradle heap to 1792m (#30484) [docs] add warning for read-write indices in force merge documentation (#28869) Avoid deadlocks in cache (#30461) Test: remove hardcoded list of unconfigured ciphers (#30367) mute SplitIndexIT due to #30416 Docs: Test examples that recreate lang analyzers (#29535) BulkProcessor to retry based on status code (#29329) Add GET Repository High Level REST API (#30362) add a comment explaining the need for RetryOnReplicaException on missing mappings Add `coordinating_only` node selector (#30313) Stop forking groovyc (#30471) Avoid setting connection request timeout (#30384) Use date format in `date_range` mapping before fallback to default (#29310) Watcher: Increase HttpClient parallel sent requests (#30130) # Conflicts: # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java

* 6.x: Upgrade to Lucene-7.4-snapshot-6705632810 (#30519) Remove Discovery.AckListener.onTimeout() (#30514) Build: move generated-resources to build (#30366) Reindex: Fold "with all deps" project into reindex (#30154) Isolate REST client single host tests (#30504) Remove BWC repository test (#30500) Build: Remove xpack specific run task (#30487) AwaitsFix IntegTestZipClientYamlTestSuiteIT#indices.split tests LLClient: Add setJsonEntity (#30447) [docs] add warning for read-write indices in force merge documentation (#28869) Avoid deadlocks in cache (#30461) BulkProcessor to retry based on status code (#29329) Avoid setting connection request timeout (#30384) Test: remove hardcoded list of unconfigured ciphers (#30367) Add GET Repository High Level REST API (#30362) mute SplitIndexIT due to #30416 Docs: Test examples that recreate lang analyzers (#29535) add a comment explaining the need for RetryOnReplicaException on missing mappings Pass the task to broadcast actions (#29672) Stop forking groovyc (#30471) Add `coordinating_only` node selector (#30313) Fix accidental error in changelog Use date format in `date_range` mapping before fallback to default (#29310) Watcher: Increase HttpClient parallel sent requests (#30130) [Security][Tests] Azeri(Turkish) locale tripps opensaml dependency

hub-cap added >feature v7.0.0 :Core/Java High Level REST Client v6.4.0 labels May 3, 2018

javanna mentioned this pull request May 3, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

Merge remote-tracking branch 'upstream/master' into repository_get_repo

a403e99

javanna requested changes May 3, 2018

View reviewed changes

hub-cap added 2 commits May 7, 2018 11:57

Merge remote-tracking branch 'upstream/master' into repository_get_repo

1a6e132

Fixups for PR review

5d1631f

Namespace to snapshot

ffa5a26

hub-cap added 3 commits May 8, 2018 07:41

Add docs for snapshots

85d56d5

Add changelog

27caf67

Merge remote-tracking branch 'upstream/master' into repository_get_repo

797b4ed

hub-cap requested a review from javanna May 8, 2018 12:57

hub-cap added >enhancement and removed >feature labels May 8, 2018

nik9000 approved these changes May 8, 2018

View reviewed changes

hub-cap added 3 commits May 8, 2018 10:39

Merge remote-tracking branch 'upstream/master' into repository_get_repo

8aef412

Fix checkstyle error

36ab3de

Remove changelog entry

662733a

javanna approved these changes May 8, 2018

View reviewed changes

remove repo param, cleanup test

4fd5c19

Merge remote-tracking branch 'upstream/master' into repository_get_repo

97b32f4

hub-cap merged commit 3b9c820 into elastic:master May 9, 2018

hub-cap added the backport pending label May 9, 2018

hub-cap removed the backport pending label May 9, 2018

jimczi 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

Add GET Repository High Level REST API #30362

Add GET Repository High Level REST API #30362

hub-cap commented May 3, 2018

elasticmachine commented May 3, 2018

javanna left a comment

javanna May 3, 2018

hub-cap May 3, 2018

javanna May 3, 2018

hub-cap May 3, 2018

javanna May 3, 2018

hub-cap May 3, 2018

javanna May 3, 2018

hub-cap May 3, 2018

javanna May 3, 2018

hub-cap May 3, 2018

javanna May 4, 2018

hub-cap commented May 3, 2018

hub-cap commented May 7, 2018

hub-cap commented May 7, 2018

hub-cap commented May 8, 2018

hub-cap commented May 8, 2018

hub-cap commented May 8, 2018

nik9000 left a comment

javanna left a comment

javanna May 8, 2018

hub-cap May 9, 2018

javanna May 8, 2018

javanna May 8, 2018

javanna May 8, 2018

javanna May 8, 2018

hub-cap May 9, 2018

hub-cap commented May 9, 2018

Add GET Repository High Level REST API #30362

Add GET Repository High Level REST API #30362

Conversation

hub-cap commented May 3, 2018

elasticmachine commented May 3, 2018

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hub-cap commented May 3, 2018

hub-cap commented May 7, 2018

hub-cap commented May 7, 2018

hub-cap commented May 8, 2018

hub-cap commented May 8, 2018

hub-cap commented May 8, 2018

nik9000 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

hub-cap commented May 9, 2018