Add PUT Repository High Level REST API by hub-cap · Pull Request #30501 · elastic/elasticsearch

hub-cap · 2018-05-10T00:00:51Z

This commit adds PUT Repository, the associated docs and tests for the
high level REST API client. A few small changes to the PutRepository
Request and Response went into the commit as well.

Relates #27205

This commit adds PUT Repository, the associated docs and tests for the high level REST API client. A few small changes to the PutRepository Request and Response went into the commit as well. Relates elastic#27205

elasticmachine · 2018-05-10T00:00:52Z

Pinging @elastic/es-core-infra

nik9000

I like it!

nik9000 · 2018-05-11T15:26:44Z

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

+     * API on elastic.co</a>
+     */
+    public void putRepositoryAsync(PutRepositoryRequest putRepositoryRequest,
+                                   ActionListener<PutRepositoryResponse> listener, Header... headers){


Add a space after ) and before { please!

javanna

thanks @hub-cap I left some comments!

javanna · 2018-05-11T15:19:01Z

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

+        parameters.withMasterTimeout(putRepositoryRequest.masterNodeTimeout());
+        parameters.withTimeout(putRepositoryRequest.timeout());
+
+        request.setEntity(createEntity(putRepositoryRequest, REQUEST_BODY_CONTENT_TYPE));


I see in the corresponding REST action that we also support the verify parameter. I guess we should support it here too?

yea definitely, ty for pointing it out. Cant believe i missed it!

javanna · 2018-05-11T15:19:21Z

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-11T15:20:13Z

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

+        String repository = "repo";
+        PutRepositoryRequest putRepositoryRequest = new PutRepositoryRequest(repository);
+        putRepositoryRequest.type(FsRepository.TYPE);
+        putRepositoryRequest.settings("{\"location\": \".\"}", XContentType.JSON);


would it be possible to randomize the values that the request holds like in other tests?

wrt this, should i be putting different types of plugins as well? Or just use the FS plugin? The other plugins would have to be loaded in gradle (which im sure is super easy), but I just want to make sure I have the scope here correctly. Also, putting other values for the location that are not approved by the config will result in exceptions, this is why i chose that generic thing. Just trying to test that its accepted and comes back w what i want.

I assumed that there is no problem setting values and checking that the output of the conversion from high-level request to low-level request is the expected one. We don't validate etc. I would do only what is straight-forward.

javanna · 2018-05-11T15:24:33Z

...main/java/org/elasticsearch/action/admin/cluster/repositories/put/PutRepositoryResponse.java


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


can you add a test for this class although very simple?

javanna · 2018-05-11T15:24:42Z

.../main/java/org/elasticsearch/action/admin/cluster/repositories/put/PutRepositoryRequest.java

+        builder.endObject();
+
+        builder.endObject();
+        return builder;


can you add a unit test for this method?

javanna · 2018-05-11T15:26:59Z

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>
+     */
+    public void putRepositoryAsync(PutRepositoryRequest putRepositoryRequest,


in the spec we call this "snapshot.create_repository" . Could you rename the methods to createRepository please?

javanna · 2018-05-11T15:27:07Z

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

        assertThat(expectedParams, equalTo(request.getParameters()));
    }

+    public void testPutRepository() throws IOException {


testCreateRepository?

javanna · 2018-05-11T15:27:26Z

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

-    private static final String testRepository = "test_repository";
+    private static final String repositoryName = "test_repository";
+
+    public void testSnapshotPutRepository() throws IOException {


testSnapshotCreateRepository?

javanna · 2018-05-11T15:27:36Z

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

+        assertTrue(acknowledged);
+    }
+
+    public void testSnapshotPutRepositoryAsync() throws InterruptedException {


testSnapshotCreateRepositoryAsync

javanna · 2018-05-11T15:28:21Z

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

@@ -0,0 +1,139 @@
+[[java-rest-high-snapshot-put-repository]]
+=== Snapshot Put Repository API


in the docs as well, can you rename put repository to create repository. We leave requests and responses as they are as we reuse them from the transport client, but for the rest we should follow the spec naming like other language clients already do.

I went ahead and changed it in all the headings and links (eg, [[java-rest-high-snapshot-create-repository]])

rename put to create add verify cleanup spacing

hub-cap · 2018-05-15T13:37:07Z

@javanna ive addressed comments, pls review. I have also added repository request/response tests so i can cover most of the new things im adding to these request/responses in this and subsequent PRs.

javanna · 2018-05-15T13:41:45Z

...c/test/java/org/elasticsearch/action/admin/cluster/repositories/RepositoryResponseTests.java

+
+import static org.hamcrest.Matchers.equalTo;
+
+public class RepositoryResponseTests extends ESTestCase {


could this extend AbstractXContentTestCase or AbstractStreamableXContentTestCase ?

indeed it can, and it makes it much simple. TYVM for showing me this test class!!!

javanna

thanks @hub-cap left one comment on testing, LGTM otherwise. feel free to push once addressed

lcawl · 2018-05-16T19:39:18Z

I think the docs/java-rest/high-level/snapshot/create_repository.asciidoc is missing from the 6.x branch. Is that intentional? It's causing some documentation build issues.

This commit adds Create Repository, the associated docs and tests for the high level REST API client. A few small changes to the PutRepository Request and Response went into the commit as well.

hub-cap · 2018-05-16T19:43:00Z

@lcawl I had not yet pushed it. I was in backport hell for a while today. I pushed it just now.

* 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) ...

Add PUT Repository High Level REST API

69e303e

This commit adds PUT Repository, the associated docs and tests for the high level REST API client. A few small changes to the PutRepository Request and Response went into the commit as well. Relates elastic#27205

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

hub-cap added the review label May 10, 2018

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

b5fba6c

javanna mentioned this pull request May 10, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

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

de4b6bc

nik9000 approved these changes May 11, 2018

View reviewed changes

javanna suggested changes May 11, 2018

View reviewed changes

hub-cap added 3 commits May 14, 2018 13:35

review comments

7524ae2

rename put to create add verify cleanup spacing

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

ad66b1c

Adding some randomization to the test

febaf55

hub-cap requested a review from javanna May 15, 2018 13:37

javanna reviewed May 15, 2018

View reviewed changes

javanna approved these changes May 15, 2018

View reviewed changes

hub-cap added 2 commits May 15, 2018 10:36

Last small cleanup + build fix

83d5490

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

046b58f

hub-cap merged commit b94bc70 into elastic:master May 16, 2018

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

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

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

		@@ -0,0 +1,139 @@
		[[java-rest-high-snapshot-put-repository]]
		=== Snapshot Put Repository API


		import static org.hamcrest.Matchers.equalTo;

		public class RepositoryResponseTests extends ESTestCase {

Conversation

hub-cap commented May 10, 2018

Uh oh!

elasticmachine commented May 10, 2018

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hub-cap commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

lcawl commented May 16, 2018

Uh oh!

hub-cap commented May 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants