From 65a35c985c6f8ed018145f238313e8b4da4d9236 Mon Sep 17 00:00:00 2001 From: Adrien Grand Date: Thu, 28 Mar 2019 09:32:09 +0100 Subject: [PATCH] Remove type from VersionConflictEngineException. (#37490) (#40514) It initially mentioned the type in the exception because the type used to be required to uniquely identify a document. This is not necessary anymore given that indices have at most one type. --- .../test/java/org/elasticsearch/client/CrudIT.java | 10 +++++----- .../reindex/AsyncBulkByScrollActionTests.java | 2 +- .../index/reindex/ReindexFailureTests.java | 2 +- .../test/delete_by_query/10_basic.yml | 4 ++-- .../rest-api-spec/test/reindex/10_basic.yml | 2 +- .../test/update_by_query/10_basic.yml | 4 ++-- .../org/elasticsearch/index/engine/Engine.java | 4 ++-- .../elasticsearch/index/engine/InternalEngine.java | 12 ++++++------ .../engine/VersionConflictEngineException.java | 14 +++++++------- .../action/bulk/TransportShardBulkActionTests.java | 4 ++-- .../java/org/elasticsearch/get/GetActionIT.java | 8 ++++---- 11 files changed, 33 insertions(+), 33 deletions(-) diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java index d5d59b07f55..6c161444e24 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java @@ -118,7 +118,7 @@ public class CrudIT extends ESRestHighLevelClientTestCase { ElasticsearchException exception = expectThrows(ElasticsearchException.class, () -> execute(deleteRequest, highLevelClient()::delete, highLevelClient()::deleteAsync)); assertEquals(RestStatus.CONFLICT, exception.status()); - assertEquals("Elasticsearch exception [type=version_conflict_engine_exception, reason=[_doc][" + docId + "]: " + + assertEquals("Elasticsearch exception [type=version_conflict_engine_exception, reason=[" + docId + "]: " + "version conflict, required seqNo [2], primary term [2]. current document has seqNo [3] and primary term [1]]", exception.getMessage()); assertEquals("index", exception.getMetadata("es.index").get(0)); @@ -147,7 +147,7 @@ public class CrudIT extends ESRestHighLevelClientTestCase { execute(deleteRequest, highLevelClient()::delete, highLevelClient()::deleteAsync); }); assertEquals(RestStatus.CONFLICT, exception.status()); - assertEquals("Elasticsearch exception [type=version_conflict_engine_exception, reason=[_doc][" + + assertEquals("Elasticsearch exception [type=version_conflict_engine_exception, reason=[" + docId + "]: version conflict, current version [12] is higher or equal to the one provided [10]]", exception.getMessage()); assertEquals("index", exception.getMetadata("es.index").get(0)); } @@ -282,7 +282,7 @@ public class CrudIT extends ESRestHighLevelClientTestCase { ElasticsearchException exception = expectThrows(ElasticsearchException.class, () -> execute(getRequest, highLevelClient()::get, highLevelClient()::getAsync)); assertEquals(RestStatus.CONFLICT, exception.status()); - assertEquals("Elasticsearch exception [type=version_conflict_engine_exception, " + "reason=[_doc][id]: " + + assertEquals("Elasticsearch exception [type=version_conflict_engine_exception, " + "reason=[id]: " + "version conflict, current version [1] is different than the one provided [2]]", exception.getMessage()); assertEquals("index", exception.getMetadata("es.index").get(0)); } @@ -508,7 +508,7 @@ public class CrudIT extends ESRestHighLevelClientTestCase { execute(wrongRequest, highLevelClient()::index, highLevelClient()::indexAsync); }); assertEquals(RestStatus.CONFLICT, exception.status()); - assertEquals("Elasticsearch exception [type=version_conflict_engine_exception, reason=[_doc][id]: " + + assertEquals("Elasticsearch exception [type=version_conflict_engine_exception, reason=[id]: " + "version conflict, required seqNo [1], primary term [5]. current document has seqNo [2] and primary term [1]]", exception.getMessage()); assertEquals("index", exception.getMetadata("es.index").get(0)); @@ -555,7 +555,7 @@ public class CrudIT extends ESRestHighLevelClientTestCase { }); assertEquals(RestStatus.CONFLICT, exception.status()); - assertEquals("Elasticsearch exception [type=version_conflict_engine_exception, reason=[_doc][with_create_op_type]: " + + assertEquals("Elasticsearch exception [type=version_conflict_engine_exception, reason=[with_create_op_type]: " + "version conflict, document already exists (current version [1])]", exception.getMessage()); } } diff --git a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/AsyncBulkByScrollActionTests.java b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/AsyncBulkByScrollActionTests.java index 92b2180f4da..bdedc65b7a6 100644 --- a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/AsyncBulkByScrollActionTests.java +++ b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/AsyncBulkByScrollActionTests.java @@ -276,7 +276,7 @@ public class AsyncBulkByScrollActionTests extends ESTestCase { versionConflicts++; responses[i] = new BulkItemResponse(i, randomFrom(DocWriteRequest.OpType.values()), new Failure(shardId.getIndexName(), "type", "id" + i, - new VersionConflictEngineException(shardId, "type", "id", "test"))); + new VersionConflictEngineException(shardId, "id", "test"))); continue; } boolean createdResponse; diff --git a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/ReindexFailureTests.java b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/ReindexFailureTests.java index c077c992beb..917d196b6e9 100644 --- a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/ReindexFailureTests.java +++ b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/ReindexFailureTests.java @@ -81,7 +81,7 @@ public class ReindexFailureTests extends ReindexTestCase { BulkByScrollResponse response = copy.get(); assertThat(response, matcher().batches(1).versionConflicts(1).failures(1).created(99)); for (Failure failure: response.getBulkFailures()) { - assertThat(failure.getMessage(), containsString("VersionConflictEngineException[[_doc][")); + assertThat(failure.getMessage(), containsString("VersionConflictEngineException[[")); } } diff --git a/modules/reindex/src/test/resources/rest-api-spec/test/delete_by_query/10_basic.yml b/modules/reindex/src/test/resources/rest-api-spec/test/delete_by_query/10_basic.yml index dd29e7701ba..d11f160bcf5 100644 --- a/modules/reindex/src/test/resources/rest-api-spec/test/delete_by_query/10_basic.yml +++ b/modules/reindex/src/test/resources/rest-api-spec/test/delete_by_query/10_basic.yml @@ -129,7 +129,7 @@ - match: {failures.0.status: 409} - match: {failures.0.cause.type: version_conflict_engine_exception} # Use a regex so we don't mind if the current version isn't always 1. Sometimes it comes out 2. - - match: {failures.0.cause.reason: "/\\[_doc\\]\\[1\\]:.version.conflict,.current.version.\\[\\d+\\].is.different.than.the.one.provided.\\[\\d+\\]/"} + - match: {failures.0.cause.reason: "/\\[1\\]:.version.conflict,.current.version.\\[\\d+\\].is.different.than.the.one.provided.\\[\\d+\\]/"} - match: {failures.0.cause.shard: /\d+/} - match: {failures.0.cause.index: test} - gte: { took: 0 } @@ -185,7 +185,7 @@ - match: {failures.0.id: "1"} - match: {failures.0.status: 409} - match: {failures.0.cause.type: version_conflict_engine_exception} - - match: {failures.0.cause.reason: "/\\[_doc\\]\\[1\\]:.version.conflict,.required.seqNo.\\[\\d+\\]/"} + - match: {failures.0.cause.reason: "/\\[1\\]:.version.conflict,.required.seqNo.\\[\\d+\\]/"} - match: {failures.0.cause.shard: /\d+/} - match: {failures.0.cause.index: test} - gte: { took: 0 } diff --git a/modules/reindex/src/test/resources/rest-api-spec/test/reindex/10_basic.yml b/modules/reindex/src/test/resources/rest-api-spec/test/reindex/10_basic.yml index 312a88ace5e..9ef6c1a90c4 100644 --- a/modules/reindex/src/test/resources/rest-api-spec/test/reindex/10_basic.yml +++ b/modules/reindex/src/test/resources/rest-api-spec/test/reindex/10_basic.yml @@ -160,7 +160,7 @@ - match: {failures.0.status: 409} - match: {failures.0.cause.type: version_conflict_engine_exception} # Use a regex so we don't mind if the version isn't always 1. Sometimes it comes out 2. - - match: {failures.0.cause.reason: "/\\[_doc\\]\\[1\\]:.version.conflict,.document.already.exists.\\(current.version.\\[\\d+\\]\\)/"} + - match: {failures.0.cause.reason: "/\\[1\\]:.version.conflict,.document.already.exists.\\(current.version.\\[\\d+\\]\\)/"} - match: {failures.0.cause.shard: /\d+/} - match: {failures.0.cause.index: dest} - gte: { took: 0 } diff --git a/modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/10_basic.yml b/modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/10_basic.yml index 15bc62214eb..08c8465c409 100644 --- a/modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/10_basic.yml +++ b/modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/10_basic.yml @@ -109,7 +109,7 @@ - match: {failures.0.status: 409} - match: {failures.0.cause.type: version_conflict_engine_exception} # Use a regex so we don't mind if the current version isn't always 1. Sometimes it comes out 2. - - match: {failures.0.cause.reason: "/\\[_doc\\]\\[1\\]:.version.conflict,.current.version.\\[\\d+\\].is.different.than.the.one.provided.\\[\\d+\\]/"} + - match: {failures.0.cause.reason: "/\\[1\\]:.version.conflict,.current.version.\\[\\d+\\].is.different.than.the.one.provided.\\[\\d+\\]/"} - match: {failures.0.cause.shard: /\d+/} - match: {failures.0.cause.index: test} - gte: { took: 0 } @@ -151,7 +151,7 @@ - match: {failures.0.id: "1"} - match: {failures.0.status: 409} - match: {failures.0.cause.type: version_conflict_engine_exception} - - match: {failures.0.cause.reason: "/\\[_doc\\]\\[1\\]:.version.conflict,.required.seqNo.\\[\\d+\\]/"} + - match: {failures.0.cause.reason: "/\\[1\\]:.version.conflict,.required.seqNo.\\[\\d+\\]/"} - match: {failures.0.cause.shard: /\d+/} - match: {failures.0.cause.index: test} - gte: { took: 0 } diff --git a/server/src/main/java/org/elasticsearch/index/engine/Engine.java b/server/src/main/java/org/elasticsearch/index/engine/Engine.java index edea3952ce4..9bed93c3716 100644 --- a/server/src/main/java/org/elasticsearch/index/engine/Engine.java +++ b/server/src/main/java/org/elasticsearch/index/engine/Engine.java @@ -633,14 +633,14 @@ public abstract class Engine implements Closeable { if (docIdAndVersion != null) { if (get.versionType().isVersionConflictForReads(docIdAndVersion.version, get.version())) { Releasables.close(searcher); - throw new VersionConflictEngineException(shardId, get.type(), get.id(), + throw new VersionConflictEngineException(shardId, get.id(), get.versionType().explainConflictForReads(docIdAndVersion.version, get.version())); } if (get.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO && ( get.getIfSeqNo() != docIdAndVersion.seqNo || get.getIfPrimaryTerm() != docIdAndVersion.primaryTerm )) { Releasables.close(searcher); - throw new VersionConflictEngineException(shardId, get.type(), get.id(), + throw new VersionConflictEngineException(shardId, get.id(), get.getIfSeqNo(), get.getIfPrimaryTerm(), docIdAndVersion.seqNo, docIdAndVersion.primaryTerm); } } diff --git a/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java b/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java index b9802190a13..69e800ef53d 100644 --- a/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java +++ b/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java @@ -619,13 +619,13 @@ public class InternalEngine extends Engine { return GetResult.NOT_EXISTS; } if (get.versionType().isVersionConflictForReads(versionValue.version, get.version())) { - throw new VersionConflictEngineException(shardId, get.type(), get.id(), + throw new VersionConflictEngineException(shardId, get.id(), get.versionType().explainConflictForReads(versionValue.version, get.version())); } if (get.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO && ( get.getIfSeqNo() != versionValue.seqNo || get.getIfPrimaryTerm() != versionValue.term )) { - throw new VersionConflictEngineException(shardId, get.type(), get.id(), + throw new VersionConflictEngineException(shardId, get.id(), get.getIfSeqNo(), get.getIfPrimaryTerm(), versionValue.seqNo, versionValue.term); } if (get.isReadFromTranslog()) { @@ -1007,13 +1007,13 @@ public class InternalEngine extends Engine { currentNotFoundOrDeleted = versionValue.isDelete(); } if (index.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO && versionValue == null) { - final VersionConflictEngineException e = new VersionConflictEngineException(shardId, index.type(), index.id(), + final VersionConflictEngineException e = new VersionConflictEngineException(shardId, index.id(), index.getIfSeqNo(), index.getIfPrimaryTerm(), SequenceNumbers.UNASSIGNED_SEQ_NO, 0); plan = IndexingStrategy.skipDueToVersionConflict(e, currentNotFoundOrDeleted, currentVersion, getPrimaryTerm()); } else if (index.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO && ( versionValue.seqNo != index.getIfSeqNo() || versionValue.term != index.getIfPrimaryTerm() )) { - final VersionConflictEngineException e = new VersionConflictEngineException(shardId, index.type(), index.id(), + final VersionConflictEngineException e = new VersionConflictEngineException(shardId, index.id(), index.getIfSeqNo(), index.getIfPrimaryTerm(), versionValue.seqNo, versionValue.term); plan = IndexingStrategy.skipDueToVersionConflict(e, currentNotFoundOrDeleted, currentVersion, getPrimaryTerm()); } else if (index.versionType().isVersionConflictForWrites( @@ -1338,13 +1338,13 @@ public class InternalEngine extends Engine { } final DeletionStrategy plan; if (delete.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO && versionValue == null) { - final VersionConflictEngineException e = new VersionConflictEngineException(shardId, delete.type(), delete.id(), + final VersionConflictEngineException e = new VersionConflictEngineException(shardId, delete.id(), delete.getIfSeqNo(), delete.getIfPrimaryTerm(), SequenceNumbers.UNASSIGNED_SEQ_NO, 0); plan = DeletionStrategy.skipDueToVersionConflict(e, currentVersion, getPrimaryTerm(), currentlyDeleted); } else if (delete.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO && ( versionValue.seqNo != delete.getIfSeqNo() || versionValue.term != delete.getIfPrimaryTerm() )) { - final VersionConflictEngineException e = new VersionConflictEngineException(shardId, delete.type(), delete.id(), + final VersionConflictEngineException e = new VersionConflictEngineException(shardId, delete.id(), delete.getIfSeqNo(), delete.getIfPrimaryTerm(), versionValue.seqNo, versionValue.term); plan = DeletionStrategy.skipDueToVersionConflict(e, currentVersion, getPrimaryTerm(), currentlyDeleted); } else if (delete.versionType().isVersionConflictForWrites(currentVersion, delete.version(), currentlyDeleted)) { diff --git a/server/src/main/java/org/elasticsearch/index/engine/VersionConflictEngineException.java b/server/src/main/java/org/elasticsearch/index/engine/VersionConflictEngineException.java index 357c9c10783..0f6c217409c 100644 --- a/server/src/main/java/org/elasticsearch/index/engine/VersionConflictEngineException.java +++ b/server/src/main/java/org/elasticsearch/index/engine/VersionConflictEngineException.java @@ -28,25 +28,25 @@ import java.io.IOException; public class VersionConflictEngineException extends EngineException { public VersionConflictEngineException(ShardId shardId, Engine.Operation op, long currentVersion, boolean deleted) { - this(shardId, op.type(), op.id(), op.versionType().explainConflictForWrites(currentVersion, op.version(), deleted)); + this(shardId, op.id(), op.versionType().explainConflictForWrites(currentVersion, op.version(), deleted)); } - public VersionConflictEngineException(ShardId shardId, String type, String id, + public VersionConflictEngineException(ShardId shardId, String id, long compareAndWriteSeqNo, long compareAndWriteTerm, long currentSeqNo, long currentTerm) { - this(shardId, type, id, "required seqNo [" + compareAndWriteSeqNo + "], primary term [" + compareAndWriteTerm +"]." + + this(shardId, id, "required seqNo [" + compareAndWriteSeqNo + "], primary term [" + compareAndWriteTerm +"]." + (currentSeqNo == SequenceNumbers.UNASSIGNED_SEQ_NO ? " but no document was found" : " current document has seqNo [" + currentSeqNo + "] and primary term ["+ currentTerm + "]" )); } - public VersionConflictEngineException(ShardId shardId, String type, String id, String explanation) { - this(shardId, null, type, id, explanation); + public VersionConflictEngineException(ShardId shardId, String id, String explanation) { + this(shardId, null, id, explanation); } - public VersionConflictEngineException(ShardId shardId, Throwable cause, String type, String id, String explanation) { - this(shardId, "[{}][{}]: version conflict, {}", cause, type, id, explanation); + public VersionConflictEngineException(ShardId shardId, Throwable cause, String id, String explanation) { + this(shardId, "[{}]: version conflict, {}", cause, id, explanation); } public VersionConflictEngineException(ShardId shardId, String msg, Throwable cause, Object... params) { diff --git a/server/src/test/java/org/elasticsearch/action/bulk/TransportShardBulkActionTests.java b/server/src/test/java/org/elasticsearch/action/bulk/TransportShardBulkActionTests.java index 8fa8060b385..610a72de6ec 100644 --- a/server/src/test/java/org/elasticsearch/action/bulk/TransportShardBulkActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/bulk/TransportShardBulkActionTests.java @@ -550,7 +550,7 @@ public class TransportShardBulkActionTests extends IndexShardTestCase { IndexRequest updateResponse = new IndexRequest("index", "_doc", "id").source(Requests.INDEX_CONTENT_TYPE, "field", "value"); - Exception err = new VersionConflictEngineException(shardId, "_doc", "id", + Exception err = new VersionConflictEngineException(shardId, "id", "I'm conflicted <(;_;)>"); Engine.IndexResult indexResult = new Engine.IndexResult(err, 0, 0, 0); IndexShard shard = mock(IndexShard.class); @@ -784,7 +784,7 @@ public class TransportShardBulkActionTests extends IndexShardTestCase { IndexRequest updateResponse = new IndexRequest("index", "_doc", "id").source(Requests.INDEX_CONTENT_TYPE, "field", "value"); - Exception err = new VersionConflictEngineException(shardId, "_doc", "id", + Exception err = new VersionConflictEngineException(shardId, "id", "I'm conflicted <(;_;)>"); Engine.IndexResult conflictedResult = new Engine.IndexResult(err, 0, 0); Engine.IndexResult mappingUpdate = diff --git a/server/src/test/java/org/elasticsearch/get/GetActionIT.java b/server/src/test/java/org/elasticsearch/get/GetActionIT.java index 77303995f74..8be9a991d17 100644 --- a/server/src/test/java/org/elasticsearch/get/GetActionIT.java +++ b/server/src/test/java/org/elasticsearch/get/GetActionIT.java @@ -441,7 +441,7 @@ public class GetActionIT extends ESIntegTestCase { assertThat(response.getResponses()[1].getResponse().getSourceAsMap().get("field").toString(), equalTo("value1")); assertThat(response.getResponses()[2].getFailure(), notNullValue()); assertThat(response.getResponses()[2].getFailure().getId(), equalTo("1")); - assertThat(response.getResponses()[2].getFailure().getMessage(), startsWith("[type1][1]: version conflict")); + assertThat(response.getResponses()[2].getFailure().getMessage(), startsWith("[1]: version conflict")); assertThat(response.getResponses()[2].getFailure().getFailure(), instanceOf(VersionConflictEngineException.class)); //Version from Lucene index @@ -464,7 +464,7 @@ public class GetActionIT extends ESIntegTestCase { assertThat(response.getResponses()[1].getResponse().getSourceAsMap().get("field").toString(), equalTo("value1")); assertThat(response.getResponses()[2].getFailure(), notNullValue()); assertThat(response.getResponses()[2].getFailure().getId(), equalTo("1")); - assertThat(response.getResponses()[2].getFailure().getMessage(), startsWith("[type1][1]: version conflict")); + assertThat(response.getResponses()[2].getFailure().getMessage(), startsWith("[1]: version conflict")); assertThat(response.getResponses()[2].getFailure().getFailure(), instanceOf(VersionConflictEngineException.class)); @@ -489,7 +489,7 @@ public class GetActionIT extends ESIntegTestCase { assertThat(response.getResponses()[1].getFailure(), notNullValue()); assertThat(response.getResponses()[1].getFailure().getId(), equalTo("2")); assertThat(response.getResponses()[1].getIndex(), equalTo("test")); - assertThat(response.getResponses()[1].getFailure().getMessage(), startsWith("[type1][2]: version conflict")); + assertThat(response.getResponses()[1].getFailure().getMessage(), startsWith("[2]: version conflict")); assertThat(response.getResponses()[2].getId(), equalTo("2")); assertThat(response.getResponses()[2].getIndex(), equalTo("test")); assertThat(response.getResponses()[2].getFailure(), nullValue()); @@ -515,7 +515,7 @@ public class GetActionIT extends ESIntegTestCase { assertThat(response.getResponses()[1].getFailure(), notNullValue()); assertThat(response.getResponses()[1].getFailure().getId(), equalTo("2")); assertThat(response.getResponses()[1].getIndex(), equalTo("test")); - assertThat(response.getResponses()[1].getFailure().getMessage(), startsWith("[type1][2]: version conflict")); + assertThat(response.getResponses()[1].getFailure().getMessage(), startsWith("[2]: version conflict")); assertThat(response.getResponses()[2].getId(), equalTo("2")); assertThat(response.getResponses()[2].getIndex(), equalTo("test")); assertThat(response.getResponses()[2].getFailure(), nullValue());