review comments

elastic · Sep 30, 2019 · 3a31040 · 3a31040
1 parent d0b1696
commit 3a31040
Show file tree

Hide file tree

Showing 6 changed files with 28 additions and 17 deletions.
diff --git a/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/IndexingIT.java b/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/IndexingIT.java
@@ -146,7 +146,7 @@ public void testAutoIdWithOpTypeCreate() throws IOException {
                 client().performRequest(waitForGreen);
 
                 Version minNodeVersion = null;
-                Map<?, ?> response = entityAsMap(client().performRequest(new Request("GET", "_nodes/plugins")));
+                Map<?, ?> response = entityAsMap(client().performRequest(new Request("GET", "_nodes")));
                 Map<?, ?> nodes = (Map<?, ?>) response.get("nodes");
                 for (Map.Entry<?, ?> node : nodes.entrySet()) {
                     Map<?, ?> nodeInfo = (Map<?, ?>) node.getValue();

diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/bulk/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/bulk/10_basic.yml
@@ -81,19 +81,24 @@
           - create:
               _index: test
           - f: 4
+          - index:
+              _index: test
+              op_type: create
+          - f: 5
   - match: { errors: true }
   - match: { items.0.index.status: 400 }
   - match: { items.0.index.error.type: illegal_argument_exception }
   - match: { items.0.index.error.reason: if _id is specified it must not be empty }
   - match: { items.1.index.result: created }
   - match: { items.2.index.result: created }
   - match: { items.3.create.result: created }
+  - match: { items.4.create.result: created }
 
   - do:
       count:
         index: test
 
-  - match: { count: 3 }
+  - match: { count: 4 }
 
 
 ---

diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/bulk/11_basic_with_types.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/bulk/11_basic_with_types.yml
@@ -84,19 +84,24 @@
               _index: test
               _type: type
           - f: 4
+          - index:
+              _index: test
+              op_type: create
+          - f: 5
   - match: { errors: true }
   - match: { items.0.index.status: 400 }
   - match: { items.0.index.error.type: illegal_argument_exception }
   - match: { items.0.index.error.reason: if _id is specified it must not be empty }
   - match: { items.1.index.result: created }
   - match: { items.2.index.result: created }
   - match: { items.3.create.result: created }
+  - match: { items.4.create.result: created }
 
   - do:
       count:
         index: test
 
-  - match: { count: 3 }
+  - match: { count: 4 }
 
 ---
 "empty action":

diff --git a/server/src/main/java/org/elasticsearch/action/index/IndexRequest.java b/server/src/main/java/org/elasticsearch/action/index/IndexRequest.java
@@ -194,6 +194,7 @@ public ActionRequestValidationException validate() {
         if (contentType == null) {
             validationException = addValidationError("content type is missing", validationException);
         }
+        assert opType == OpType.INDEX || opType == OpType.CREATE : "unexpected op-type: " + opType;
         final long resolvedVersion = resolveVersionDefaults();
         if (opType == OpType.CREATE) {
             if (versionType != VersionType.INTERNAL) {
@@ -216,9 +217,6 @@ public ActionRequestValidationException validate() {
         }
 
         if (id == null) {
-            if (opType != OpType.INDEX && opType != OpType.CREATE) {
-                addValidationError("an id is required for a " + opType + " operation", validationException);
-            }
             if (versionType != VersionType.INTERNAL ||
                 (resolvedVersion != Versions.MATCH_DELETED && resolvedVersion != Versions.MATCH_ANY)) {
                 validationException = addValidationError("an id must be provided if version type or value are set", validationException);

diff --git a/server/src/test/java/org/elasticsearch/document/DocumentActionsIT.java b/server/src/test/java/org/elasticsearch/document/DocumentActionsIT.java
@@ -189,7 +189,7 @@ public void testBulk() throws Exception {
                 .add(client().prepareIndex().setIndex("test").setType("type1").setId("1").setSource(source("1", "test")))
                 .add(client().prepareIndex().setIndex("test").setType("type1").setId("2").setSource(source("2", "test")).setCreate(true))
                 .add(client().prepareIndex().setIndex("test").setType("type1").setSource(source("3", "test")))
-                .add(client().prepareIndex().setIndex("test").setType("type1").setCreate(true).setSource(source("3", "test")))
+                .add(client().prepareIndex().setIndex("test").setType("type1").setCreate(true).setSource(source("4", "test")))
                 .add(client().prepareDelete().setIndex("test").setType("type1").setId("1"))
                 .add(client().prepareIndex().setIndex("test").setType("type1").setSource("{ xxx }", XContentType.JSON)) // failure
                 .execute().actionGet();
@@ -250,6 +250,10 @@ public void testBulk() throws Exception {
             getResult = client().get(getRequest("test").id(generatedId3)).actionGet();
             assertThat("cycle #" + i, getResult.getSourceAsString(), equalTo(Strings.toString(source("3", "test"))));
             assertThat(getResult.getIndex(), equalTo(getConcreteIndexName()));
+
+            getResult = client().get(getRequest("test").id(generatedId4)).actionGet();
+            assertThat("cycle #" + i, getResult.getSourceAsString(), equalTo(Strings.toString(source("4", "test"))));
+            assertThat(getResult.getIndex(), equalTo(getConcreteIndexName()));
         }
     }
 

diff --git a/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java b/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java
@@ -760,14 +760,13 @@ public void testTranslogMultipleOperationsSameDocument() throws IOException {
                 final ParsedDocument doc = testParsedDocument("1", null, testDocumentWithTextField(), SOURCE, null);
                 if (randomBoolean()) {
                         final Engine.Index operation = new Engine.Index(newUid(doc), doc, UNASSIGNED_SEQ_NO,
-                        0, i, VersionType.EXTERNAL, Engine.Operation.Origin.PRIMARY, IndexRequest.UNSET_AUTO_GENERATED_TIMESTAMP, -1, false,
-                            UNASSIGNED_SEQ_NO, 0);
+                        0, i, VersionType.EXTERNAL, Engine.Operation.Origin.PRIMARY, System.nanoTime(),
+                            IndexRequest.UNSET_AUTO_GENERATED_TIMESTAMP, false, UNASSIGNED_SEQ_NO, 0);
                     operations.add(operation);
                     initialEngine.index(operation);
                 } else {
                     final Engine.Delete operation = new Engine.Delete("test", "1", newUid(doc), UNASSIGNED_SEQ_NO, 0, i,
-                        VersionType.EXTERNAL, Engine.Operation.Origin.PRIMARY, IndexRequest.UNSET_AUTO_GENERATED_TIMESTAMP,
-                        UNASSIGNED_SEQ_NO, 0);
+                        VersionType.EXTERNAL, Engine.Operation.Origin.PRIMARY, System.nanoTime(), UNASSIGNED_SEQ_NO, 0);
                     operations.add(operation);
                     initialEngine.delete(operation);
                 }
@@ -3640,8 +3639,8 @@ public void testRetryWithAutogeneratedIdWorksAndNoDuplicateDocs() throws IOExcep
         long autoGeneratedIdTimestamp = 0;
 
         Engine.Index index = new Engine.Index(newUid(doc), doc, UNASSIGNED_SEQ_NO, 0,
-            Versions.MATCH_DELETED, VersionType.INTERNAL, PRIMARY, System.nanoTime(), autoGeneratedIdTimestamp, isRetry,
-            UNASSIGNED_SEQ_NO, 0);
+            randomBoolean() ? Versions.MATCH_DELETED : Versions.MATCH_ANY, VersionType.INTERNAL, PRIMARY, System.nanoTime(),
+            autoGeneratedIdTimestamp, isRetry, UNASSIGNED_SEQ_NO, 0);
         Engine.IndexResult indexResult = engine.index(index);
         assertThat(indexResult.getVersion(), equalTo(1L));
 
@@ -3680,8 +3679,8 @@ public void testRetryWithAutogeneratedIdsAndWrongOrderWorksAndNoDuplicateDocs()
         long autoGeneratedIdTimestamp = 0;
 
         Engine.Index firstIndexRequest = new Engine.Index(newUid(doc), doc, UNASSIGNED_SEQ_NO, 0,
-            Versions.MATCH_DELETED, VersionType.INTERNAL, PRIMARY, System.nanoTime(), autoGeneratedIdTimestamp, isRetry, UNASSIGNED_SEQ_NO,
-            0);
+            randomBoolean() ? Versions.MATCH_DELETED : Versions.MATCH_ANY, VersionType.INTERNAL, PRIMARY, System.nanoTime(),
+            autoGeneratedIdTimestamp, isRetry, UNASSIGNED_SEQ_NO, 0);
         Engine.IndexResult result = engine.index(firstIndexRequest);
         assertThat(result.getVersion(), equalTo(1L));
 
@@ -4774,7 +4773,7 @@ public void testSeqNoGenerator() throws IOException {
                     Versions.MATCH_ANY,
                     VersionType.INTERNAL,
                     Engine.Operation.Origin.PRIMARY,
-                    System.currentTimeMillis(),
+                    System.nanoTime(),
                     IndexRequest.UNSET_AUTO_GENERATED_TIMESTAMP,
                     randomBoolean(), UNASSIGNED_SEQ_NO, 0);
             final Engine.IndexResult indexResult = e.index(index);
@@ -4790,7 +4789,7 @@ public void testSeqNoGenerator() throws IOException {
                     Versions.MATCH_ANY,
                     VersionType.INTERNAL,
                     Engine.Operation.Origin.PRIMARY,
-                    IndexRequest.UNSET_AUTO_GENERATED_TIMESTAMP, UNASSIGNED_SEQ_NO, 0);
+                    System.nanoTime(), UNASSIGNED_SEQ_NO, 0);
             final Engine.DeleteResult deleteResult = e.delete(delete);
             assertThat(deleteResult.getSeqNo(), equalTo(seqNo + 1));
             assertThat(seqNoGenerator.get(), equalTo(seqNo + 2));