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

Add GET Repository High Level REST API #30362

Merged
merged 13 commits into from
May 9, 2018
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.client;

import org.apache.http.Header;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.admin.cluster.repositories.get.GetRepositoriesRequest;
import org.elasticsearch.action.admin.cluster.repositories.get.GetRepositoriesResponse;
import org.elasticsearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest;
import org.elasticsearch.action.admin.cluster.settings.ClusterUpdateSettingsResponse;

import java.io.IOException;

import static java.util.Collections.emptySet;

/**
* A wrapper for the {@link RestHighLevelClient} that provides methods for accessing the various Modules APIs.
* <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 {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see my overall comment.

private final RestHighLevelClient restHighLevelClient;

ModulesClient(RestHighLevelClient restHighLevelClient) {
this.restHighLevelClient = restHighLevelClient;
}

/**
* Gets a list of snapshot repositories. If the list of repositories is empty or it contains a single element "_all", all
* registered repositories are returned.
* <p>
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-snapshots.html"> Snapshot and Restore
* API on elastic.co</a>
*/
public GetRepositoriesResponse getRepositories(GetRepositoriesRequest getRepositoriesRequest, Header... headers)
throws IOException {
return restHighLevelClient.performRequestAndParseEntity(getRepositoriesRequest, RequestConverters::modulesGetRepositories,
GetRepositoriesResponse::fromXContent, emptySet(), headers);
}

/**
* Asynchronously gets a list of snapshot repositories. If the list of repositories is empty or it contains a single element "_all", all
* registered repositories are returned.
* <p>
* 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 getRepositoriesAsync(GetRepositoriesRequest getRepositoriesRequest,
ActionListener<GetRepositoriesResponse> listener, Header... headers) {
restHighLevelClient.performRequestAsyncAndParseEntity(getRepositoriesRequest, RequestConverters::modulesGetRepositories,
GetRepositoriesResponse::fromXContent, listener, emptySet(), headers);
}
}


Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.apache.http.entity.ContentType;
import org.apache.lucene.util.BytesRef;
import org.elasticsearch.action.DocWriteRequest;
import org.elasticsearch.action.admin.cluster.repositories.get.GetRepositoriesRequest;
import org.elasticsearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest;
import org.elasticsearch.action.admin.indices.alias.IndicesAliasesRequest;
import org.elasticsearch.action.admin.indices.alias.get.GetAliasesRequest;
Expand Down Expand Up @@ -630,6 +631,17 @@ static Request indexPutSettings(UpdateSettingsRequest updateSettingsRequest) thr
return request;
}

static Request modulesGetRepositories(GetRepositoriesRequest getRepositoriesRequest) {
Copy link
Member

Choose a reason for hiding this comment

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

could we add a test for this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ill add, for sure 👍

Request request = new Request(HttpGet.METHOD_NAME, "/_snapshot");

Params parameters = new Params(request);
parameters.withMasterTimeout(getRepositoriesRequest.masterNodeTimeout());
parameters.withLocal(getRepositoriesRequest.local());
parameters.withRepository(getRepositoriesRequest.repositories());

return request;
}

private static HttpEntity createEntity(ToXContent toXContent, XContentType xContentType) throws IOException {
BytesRef source = XContentHelper.toXContent(toXContent, xContentType, false).toBytesRef();
return new ByteArrayEntity(source.bytes, source.offset, source.length, createContentType(xContentType));
Expand Down Expand Up @@ -763,6 +775,13 @@ Params withRefreshPolicy(WriteRequest.RefreshPolicy refreshPolicy) {
return this;
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, ive fixed this. Good call.

}
return this;
}

Params withRetryOnConflict(int retryOnConflict) {
if (retryOnConflict > 0) {
return putParam("retry_on_conflict", String.valueOf(retryOnConflict));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ public class RestHighLevelClient implements Closeable {

private final IndicesClient indicesClient = new IndicesClient(this);
private final ClusterClient clusterClient = new ClusterClient(this);
private final ModulesClient modulesClient = new ModulesClient(this);

/**
* Creates a {@link RestHighLevelClient} given the low level {@link RestClientBuilder} that allows to build the
Expand Down Expand Up @@ -252,6 +253,15 @@ public final ClusterClient cluster() {
return clusterClient;
}

/**
* Provides a {@link ModulesClient} which can be used to access the various Modules APIs.
*
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/modules.html">Modules APIs on elastic.co</a>
*/
public final ModulesClient modules() {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally wasnt aware, makes sense.

return modulesClient;
}

/**
* Executes a bulk request using the Bulk API
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.client;

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.action.admin.cluster.repositories.get.GetRepositoriesRequest;
import org.elasticsearch.action.admin.cluster.repositories.get.GetRepositoriesResponse;
import org.elasticsearch.rest.RestStatus;

import java.io.IOException;

import static org.hamcrest.Matchers.equalTo;

public class ModulesClientIT extends ESRestHighLevelClientTestCase {

public void testModulesGetRepositories() throws IOException {
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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

create is the next thing im working on :)

assertThat(0, equalTo(response.repositories().size()));
}

public void testModulesGetRepositoriesNonExistent() throws IOException {
String repo = "doesnotexist";
GetRepositoriesRequest request = new GetRepositoriesRequest(new String[]{repo});
ElasticsearchException exception = expectThrows(ElasticsearchException.class, () -> execute(request,
highLevelClient().modules()::getRepositories, highLevelClient().modules()::getRepositoriesAsync));

assertThat(exception.status(), equalTo(RestStatus.NOT_FOUND));
assertThat(exception.getMessage(), equalTo(
"Elasticsearch exception [type=repository_missing_exception, reason=[" + repo + "] missing]"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@

package org.elasticsearch.action.admin.cluster.repositories.get;

import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.action.ActionResponse;
import org.elasticsearch.cluster.metadata.RepositoriesMetaData;
import org.elasticsearch.cluster.metadata.RepositoryMetaData;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.ToXContentObject;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;

import java.io.IOException;
import java.util.Collections;
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

throw new ElasticsearchParseException("failed to parse repositories, expected start object token");
}
return new GetRepositoriesResponse(RepositoriesMetaData.fromXContent(parser));
}
}