If the index does not exist, delete document will not auto create it #24518

olcbean · 2017-05-05T19:27:26Z

Currently a delete document request against a non-existing index actually creates this index.

With this change the delete document no longer creates the previously non-existing index and throws an index_not_found exception instead.

However as discussed in #15451 (comment), if an external version is explicitly used, the current behavior is preserved and the index is still created and the document is marked for deletion.

Fixes #15425

elasticmachine · 2017-05-05T19:27:28Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

bleskes

Thx @olcbean for the PR. I left some comments. We should add a section to the breaking changes documentation here

bleskes · 2017-05-12T13:28:36Z

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

+            ElasticsearchException exception = expectThrows(ElasticsearchException.class,
+                    () -> execute(deleteRequest, highLevelClient()::delete, highLevelClient()::deleteAsync));
+            assertEquals(RestStatus.NOT_FOUND, exception.status());
+            assertEquals("Elasticsearch exception [type=index_not_found_exception, reason=no such index]", exception.getMessage());


The goal of this test is to check how delete behaves where it doesn't find the document in question. I don't think there is a need to test the index creation on the rest client tests (this is done in the core testing). Just create the index and let the test what it does.

bleskes · 2017-05-12T13:37:08Z

core/src/test/java/org/elasticsearch/action/IndicesRequestIT.java

@@ -223,6 +228,18 @@ public void testDelete() {
        assertSameIndices(deleteRequest, deleteShardActions);
    }

+    // for delete index throws an IndexNotFoundException if the index does not exist or create the index if external version is used
+    public void testDeleteAction() throws InterruptedException, ExecutionException {


I think this is the wrong place for this - this suite checks that index names are passed correctly to sub request. IMO you should create a unit test TransportBulkActionTests class, which inherits from ESTestCase and test how the TransportBulkAction decides which indices need to be auto created. You can look at how TransportBulkActionIngestTests.TestTransportBulkAction is set up as an example. Let me know if you need more guidance and I'm happy to help

@bleskes thanks for the tip, I will add a TransportBulkActionTests.

Hm... It appeared to me that at the time the issue was opened, the delete was not using BulkAction ( based on some comments ). So I decided to add the test here to verify that explicitly using the delete endpoint will not create the index. But of course I can remove it ;)

bleskes · 2017-05-12T13:41:15Z

...t/java/org/elasticsearch/action/bulk/TransportBulkActionIndicesThatCannotBeCreatedTests.java

@@ -68,7 +68,7 @@ public void testAllFail() {
        bulkRequest.add(new IndexRequest("can't"));
        bulkRequest.add(new DeleteRequest("do"));


maybe change this to a delete request with an external version?

bleskes · 2017-05-12T13:41:49Z

docs/reference/docs/delete.asciidoc

@@ -105,7 +105,8 @@ thrown instead.
 [[delete-index-creation]]
 === Automatic index creation

-The delete operation automatically creates an index if it has not been
+If external version is used,


can you mention external_gte too?

What about adding a link here as there are actually 3 external version types : external, external_gt and external_gte ?

I'm fine with something generic here (if one of the external versioning variants is used) and linking to the versioning page.

unless an external versioning is used

olcbean · 2017-05-18T07:26:19Z

@bleskes thank you for your review!

I just pushed the changes you suggested.

reverting the changes to the REST client tests and the IndicesRequestIT add a section to the indices.asciidoc

bleskes

Looks great. I left some very minor requests.

bleskes · 2017-05-18T13:05:26Z

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

@@ -65,6 +64,11 @@
    public void testDelete() throws IOException {
        {
            // Testing non existing document
+            final XContentType xContentType = randomFrom(XContentType.values());


why was this added?

I could not figure out how else to explicitly create an index in the REST client... Do you have a suggestion?

I think you still need to use the low level client for that, using client().performRequest(). That said, why not change the order of the test blocks around and move the // Testing deletion block to be first. It will create the index.

Well, duh! 😊 Thanks @bleskes!

bleskes · 2017-05-18T13:07:49Z

core/src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java

@@ -144,6 +145,9 @@ protected void doExecute(Task task, BulkRequest bulkRequest, ActionListener<Bulk
            // Attempt to create all the indices that we're going to need during the bulk before we start.
            // Step 1: collect all the indices in the request
            final Set<String> indices = bulkRequest.requests.stream()
+                .filter(request -> request.opType() != DocWriteRequest.OpType.DELETE 
+                        || request.versionType() == VersionType.EXTERNAL 


can you add a comment here as to why we have a special handling for external versions and deletes here?

bleskes · 2017-05-18T13:08:10Z

core/src/test/java/org/elasticsearch/action/IndicesRequestIT.java

@@ -66,6 +66,7 @@
 import org.elasticsearch.action.index.IndexRequest;
 import org.elasticsearch.action.search.SearchRequest;
 import org.elasticsearch.action.search.SearchResponse;
+import org.elasticsearch.action.search.SearchTransportService;


nit: left over formatting.. can you please remove?

Hm... you want me revert and leave the imports not organized?

In general I agree that major reformatting should happen separately but it is just a single import ;)

I hear you. On the other hand, it's just a reset to commit away :)

bleskes · 2017-05-18T13:13:49Z

core/src/test/java/org/elasticsearch/action/bulk/TransportBulkActionTests.java

+        super.tearDown();
+    }
+
+    public void testDeleteNonExistingDoc() throws Exception {


can we call this testDeleteNonExistingDocDoesNotCreateIndices

bleskes · 2017-05-18T13:15:10Z

core/src/test/java/org/elasticsearch/action/bulk/TransportBulkActionTests.java

+        }));
+    }
+
+    public void testDeleteNonExistingDocExternalVersion() throws Exception {


same comment about naming. Can we add "CreatesIndex"?

olcbean · 2017-05-18T16:01:39Z

@bleskes thanks for the remarks!

Do you know how I can simply create an index in the REST client?
I know that what I came up with is far from ideal, but I did not see an alternative. It would be great if you can give me a pointer or idea?

bleskes · 2017-05-22T06:59:04Z

thanks a lot @olcbean . I'll merge this in.

elasticmachine · 2017-10-20T11:03:05Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

ywelsch requested a review from bleskes May 12, 2017 12:19

bleskes suggested changes May 12, 2017

View reviewed changes

If the index does not exist, delete document will not auto create it

0d51498

unless an external versioning is used

olcbean force-pushed the delete_no_index_created branch from dfa2725 to bc5efd8 Compare May 18, 2017 07:24

olcbean force-pushed the delete_no_index_created branch from bc5efd8 to c44d565 Compare May 18, 2017 07:31

creating TransportBulkActionTests

0ab6b95

reverting the changes to the REST client tests and the IndicesRequestIT add a section to the indices.asciidoc

olcbean force-pushed the delete_no_index_created branch from c44d565 to 0ab6b95 Compare May 18, 2017 07:52

bleskes suggested changes May 18, 2017

View reviewed changes

bleskes added :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. >breaking >enhancement labels May 18, 2017

integrating remarks

fa39cef

olcbean force-pushed the delete_no_index_created branch from fc60911 to fa39cef Compare May 18, 2017 15:58

correcting CrudIT

f376ac9

bleskes approved these changes May 22, 2017

View reviewed changes

bleskes added the v6.0.0 label May 22, 2017

bleskes merged commit e08e92d into elastic:master May 22, 2017

clintongormley removed the >enhancement label May 22, 2017

clintongormley added v6.0.0-alpha2 and removed v6.0.0 labels Jun 6, 2017

olcbean deleted the delete_no_index_created branch July 8, 2017 11:38

olcbean changed the title ~~Deleting a document from a non-existing index creates the indexIf the index does not exist, delete document will not auto create it~~ If the index does not exist, delete document will not auto create it Oct 20, 2017

jasontedor mentioned this pull request Oct 21, 2017

DELETE document response on missing index has changed since 6.x beta #27070

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

If the index does not exist, delete document will not auto create it #24518

If the index does not exist, delete document will not auto create it #24518

olcbean commented May 5, 2017

elasticmachine commented May 5, 2017

bleskes left a comment

bleskes May 12, 2017

bleskes May 12, 2017

olcbean May 15, 2017

bleskes May 12, 2017

bleskes May 12, 2017

olcbean May 14, 2017

bleskes May 15, 2017

olcbean commented May 18, 2017

bleskes left a comment

bleskes May 18, 2017

olcbean May 18, 2017

bleskes May 18, 2017

olcbean May 18, 2017

bleskes May 18, 2017

bleskes May 18, 2017

olcbean May 18, 2017

bleskes May 18, 2017

bleskes May 18, 2017

bleskes May 18, 2017

olcbean commented May 18, 2017

bleskes commented May 22, 2017

elasticmachine commented Oct 20, 2017

		@@ -68,7 +68,7 @@ public void testAllFail() {
		bulkRequest.add(new IndexRequest("can't"));
		bulkRequest.add(new DeleteRequest("do"));

If the index does not exist, delete document will not auto create it #24518

If the index does not exist, delete document will not auto create it #24518

Conversation

olcbean commented May 5, 2017

elasticmachine commented May 5, 2017

bleskes 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

olcbean commented May 18, 2017

bleskes 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

olcbean commented May 18, 2017

bleskes commented May 22, 2017

elasticmachine commented Oct 20, 2017