Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java
100644 → 100755
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.indices.delete.DeleteIndexRequest;
import org.elasticsearch.action.bulk.BulkRequest;
import org.elasticsearch.action.delete.DeleteRequest;
import org.elasticsearch.action.get.GetRequest;
Expand Down Expand Up @@ -123,6 +124,17 @@ static Request delete(DeleteRequest deleteRequest) {
return new Request(HttpDelete.METHOD_NAME, endpoint, parameters.getParams(), null);
}


Copy link
Member

Choose a reason for hiding this comment

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

nit: can you remove this additional empty line? one is enough between two methods.

static Request deleteIndex(DeleteIndexRequest deleteIndexRequest) {
String endpoint = endpoint(deleteIndexRequest.indices(), "");

Params parameters = Params.builder();
parameters.withTimeout(deleteIndexRequest.timeout());
parameters.withIndicesOptions(deleteIndexRequest.indicesOptions());
Copy link
Member

Choose a reason for hiding this comment

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

I think that master_timeout is ignored, can you read and set that one too?


return new Request(HttpDelete.METHOD_NAME, endpoint, parameters.getParams(), null);
}

static Request info() {
return new Request(HttpGet.METHOD_NAME, "/", Collections.emptyMap(), null);
}
Expand Down Expand Up @@ -378,6 +390,11 @@ static String endpoint(String[] indices, String[] types, String endpoint) {
return endpoint(String.join(",", indices), String.join(",", types), endpoint);
}


Copy link
Member

Choose a reason for hiding this comment

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

nit: can you remove this additional empty line? one is enough between two methods.

static String endpoint(String[] indices, String endpoint) {
Copy link
Member

Choose a reason for hiding this comment

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

can you remove the endpoint argument here, as it is not needed. We will add it later if/when we need it for other APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that it would then clash with the method below:

    /**
     * Utility method to build request's endpoint.
     */
    static String endpoint(String... parts) {
        StringJoiner joiner = new StringJoiner("/", "/", "");
        for (String part : parts) {
            if (Strings.hasLength(part)) {
                joiner.add(part);
            }
        }
        return joiner.toString();
    }

To be fair, having multiple endpoint methods that all take String collections as parameters, but have different behaviour, does feel like asking for trouble. How about renaming one or the other to buildEndpoint, buildPath, or similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, I could just not add a new method and call:

  String endpoint = endpoint(deleteIndexRequest.indices(), new String[0], "");

Would that be preferable?

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 comment thread: #27019 (comment))

Copy link
Member

Choose a reason for hiding this comment

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

thanks for your comments, how about using the existing endpoint method then and calling it like this endpoint(indices, Strings.EMPTY_ARRAY, "") ?

return endpoint(String.join(",", indices), endpoint);
}

/**
* Utility method to build request's endpoint.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a unit test method to RequestTests for the new deleteIndex method?

Expand Down
22 changes: 22 additions & 0 deletions client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.ActionRequest;
import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.action.admin.indices.delete.DeleteIndexRequest;
import org.elasticsearch.action.admin.indices.delete.DeleteIndexResponse;
import org.elasticsearch.action.bulk.BulkRequest;
import org.elasticsearch.action.bulk.BulkResponse;
import org.elasticsearch.action.delete.DeleteRequest;
Expand Down Expand Up @@ -346,6 +348,26 @@ public void deleteAsync(DeleteRequest deleteRequest, ActionListener<DeleteRespon
Collections.singleton(404), headers);
}

/**
* Deletes an index using the Delete Index api
* <p>
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-delete-index.html">Delete API on elastic.co</a>
Copy link
Member

Choose a reason for hiding this comment

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

can you replace here in the javadocs Delete API with Delete Index API?

*/
public DeleteIndexResponse deleteIndex(DeleteIndexRequest deleteIndexRequest, Header... headers) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

the end goal would be to expose these new methods as part of a different namespace, like all the other official language clients do: client.indices().deleteIndex(...) rather than client.deleteIndex(...). Could you make that change please? I guess that requires to have an IndicesClient class that holds these new methods for indices management, and a getter for it in RestHighLevelClient.

return performRequestAndParseEntity(deleteIndexRequest, Request::deleteIndex, DeleteIndexResponse::fromXContent,
Collections.singleton(404), headers);
}

/**
* Asynchronously deletes an index using the Delete Index api
* <p>
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-delete-index.html">Delete API on elastic.co</a>
Copy link
Member

Choose a reason for hiding this comment

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

same as above: Delete API -> Delete Index API?

*/
public void deleteIndexAsync(DeleteIndexRequest deleteIndexRequest, ActionListener<DeleteIndexResponse> listener, Header... headers) {
performRequestAsyncAndParseEntity(deleteIndexRequest, Request::deleteIndex, DeleteIndexResponse::fromXContent, listener,
Collections.singleton(404), headers);
Copy link
Member

Choose a reason for hiding this comment

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

the ignore set is needed for response parsing, in case an error code is returned but the corresponding response body needs to be parsed into a response object rather than an exception. That is needed for the GET API for instance, as 404 corresponds to a valid GetResponse being returned with found set to false. IN this case, 404 corresponds to an error which needs to be parsed into an exception, so I don't think we need to add 404 to the ignore set, such set should be empty. Makes sense?

}

/**
* Executes a search using the Search api
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* 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.indices.delete.DeleteIndexRequest;
import org.elasticsearch.action.admin.indices.delete.DeleteIndexResponse;
import org.elasticsearch.action.get.GetRequest;
import org.elasticsearch.action.index.IndexRequest;
import org.elasticsearch.rest.RestStatus;

import java.io.IOException;
import java.util.Collections;

public class IndexAdminIT extends ESRestHighLevelClientTestCase {
Copy link
Member

Choose a reason for hiding this comment

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

can you rename this test class to IndicesClientIT so it corresponds to the IndicesClient class that I suggest we add above?


public void testDeleteIndex_ifIndexExists() throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

I would probably merge the two methods into a single testDeleteIndex method

// Testing existing index is deleted
GetRequest getRequest = new GetRequest("test_index", "type", "id");
highLevelClient().index(new IndexRequest("test_index", "type", "id").source(Collections.singletonMap("foo", "bar")));

DeleteIndexRequest deleteIndexRequest = new DeleteIndexRequest("test_index");
DeleteIndexResponse deleteIndexResponse =
execute(deleteIndexRequest, highLevelClient()::deleteIndex, highLevelClient()::deleteIndexAsync);
assertTrue(deleteIndexResponse.isAcknowledged());

ElasticsearchException exception = expectThrows(ElasticsearchException.class,
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to have support for the indices.exists API here. The fact that get API returns 404 doesn't mean that the index is not there. I would rather call indices.exists using the low level client, it is just a HEAD request against the index and we only need to check the response code that we get back.

() -> execute(getRequest, highLevelClient()::get, highLevelClient()::getAsync));
assertEquals(RestStatus.NOT_FOUND, exception.status());
}

public void testDeleteIndex_ifIndexNotExists() throws IOException {
// Testing error on non-existing index
DeleteIndexRequest deleteIndexRequest = new DeleteIndexRequest("non_existent_index");

ElasticsearchException exception = expectThrows(ElasticsearchException.class,
() -> execute(deleteIndexRequest, highLevelClient()::deleteIndex, highLevelClient()::deleteIndexAsync));
assertEquals(RestStatus.NOT_FOUND, exception.status());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.elasticsearch.action.support.master.AcknowledgedResponse;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.XContentBuilder;

import java.io.IOException;

Expand All @@ -48,5 +49,4 @@ public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
writeAcknowledged(out);
}

Copy link
Member

Choose a reason for hiding this comment

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

can you remove this unnecessary change?

}
33 changes: 33 additions & 0 deletions core/src/main/java/org/elasticsearch/action/admin/indices/delete/DeleteIndexResponse.java
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,12 @@
import org.elasticsearch.action.support.master.AcknowledgedResponse;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.XContentParser;

import java.io.IOException;

import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;

/**
* A response for a delete index action.
*/
Expand All @@ -48,4 +51,34 @@ public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
writeAcknowledged(out);
}

public static DeleteIndexResponse fromXContent(XContentParser parser) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

We need to add unit tests for this parsing method, similar to what we did in IndexResponseTests, GetResponseTests etc.

ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation);

DeleteIndexResponse.Builder context = new DeleteIndexResponse.Builder();
while (parser.nextToken() != XContentParser.Token.END_OBJECT) {
parseXContentFields(parser, context);
}
return context.build();
}

/**
* Parse the current token and update the parsing context appropriately.
*/
private static void parseXContentFields(XContentParser parser, DeleteIndexResponse.Builder context) throws IOException {
AcknowledgedResponse.parseInnerToXContent(parser, context);
}

/**
* Builder class for {@link DeleteIndexResponse}. This builder is usually used during xcontent parsing to
* temporarily store the parsed values, then the {@link AcknowledgedResponse.Builder#build()} method is called to
* instantiate the {@link DeleteIndexResponse}.
*/
public static class Builder extends AcknowledgedResponse.Builder {

@Override
public DeleteIndexResponse build() {
return new DeleteIndexResponse(acknowledged);
}
}
}
46 changes: 45 additions & 1 deletion core/src/main/java/org/elasticsearch/action/support/master/AcknowledgedResponse.java
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,21 @@
import org.elasticsearch.action.ActionResponse;
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 static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;

/**
* Abstract class that allows to mark action responses that support acknowledgements.
* Facilitates consistency across different api.
*/
public abstract class AcknowledgedResponse extends ActionResponse {
public abstract class AcknowledgedResponse extends ActionResponse implements ToXContentObject {

private static final String ACKNOWLEDGED = "acknowledged";

private boolean acknowledged;

Expand Down Expand Up @@ -61,4 +68,41 @@ protected void readAcknowledged(StreamInput in) throws IOException {
protected void writeAcknowledged(StreamOutput out) throws IOException {
out.writeBoolean(acknowledged);
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

I think that this is a potentially good change, but should not be part of this PR as it's not really needed. Also, if we go down this route, we should move code from AcknowledgedRestListener#buildResponse to AcknowledgedResponse and change the way RestDeleteIndexAction prints the response body out, otherwise we introduce code duplication. Would you mind doing this in another PR and reverting it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry, that was just some over-enthusiastic copy & pasting.

Copy link
Member

Choose a reason for hiding this comment

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

no worries!

builder.startObject();
builder.field("acknowledged", acknowledged);
builder.endObject();
return builder;
}

protected static void parseInnerToXContent(XContentParser parser, AcknowledgedResponse.Builder context) throws IOException {
XContentParser.Token token = parser.currentToken();
Copy link
Member

Choose a reason for hiding this comment

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

given this hierarchy, maybe we should check already if we support classes that add their custom fields to the response, like CreateIndexResponse. Maybe we can add parsing code for that one too (only parsing, without adding the rest of the API)? What do you think?

ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser::getTokenLocation);

String currentFieldName = parser.currentName();
token = parser.nextToken();

if (token.isValue()) {
if (ACKNOWLEDGED.equals(currentFieldName)) {
context.setAcknowledged(parser.booleanValue());
}
}
}

public abstract static class Builder {

protected boolean acknowledged = false;

public boolean isAcknowledged() {
return acknowledged;
}

public void setAcknowledged(boolean acknowledged) {
this.acknowledged = acknowledged;
}

public abstract AcknowledgedResponse build();
}
}