From 62dcc5b1ae1b7a7e31d347c4c9d7936a9fef747b Mon Sep 17 00:00:00 2001
From: David Turner <david.turner@elastic.co>
Date: Thu, 17 Sep 2020 09:25:22 +0100
Subject: [PATCH] Suppress stack in VersionConflictEngineException (#62433)

`VersionConflictEngineException` is thrown on the hot path for updates,
but stack traces are expensive to compute and transport and rarely
useful for this kind of exception. This commit avoids computing the
stack trace for these exceptions.
---
 .../index/engine/VersionConflictEngineException.java   |  6 ++++++
 .../index/engine/InternalEngineTests.java              | 10 +++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

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 c869e2bc386..be854b11778 100644
--- a/server/src/main/java/org/elasticsearch/index/engine/VersionConflictEngineException.java
+++ b/server/src/main/java/org/elasticsearch/index/engine/VersionConflictEngineException.java
@@ -57,4 +57,10 @@ public class VersionConflictEngineException extends EngineException {
     public VersionConflictEngineException(StreamInput in) throws IOException {
         super(in);
     }
+
+    @Override
+    public synchronized Throwable fillInStackTrace() {
+        // This is on the hot path for updates; stack traces are expensive to compute and not very useful for VCEEs, so don't fill it in.
+        return this;
+    }
 }
diff --git a/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java b/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java
index d096362edf0..b9cf12d5b8c 100644
--- a/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java
+++ b/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java
@@ -195,6 +195,7 @@ import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.empty;
+import static org.hamcrest.Matchers.emptyArray;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.everyItem;
 import static org.hamcrest.Matchers.greaterThan;
@@ -1476,9 +1477,11 @@ public class InternalEngineTests extends EngineTestCase {
                 .setIfSeqNo(indexResult.getSeqNo()).setIfPrimaryTerm(primaryTerm.get() + 1),
             searcherFactory));
 
-        expectThrows(VersionConflictEngineException.class, () -> engine.get(new Engine.Get(true, false, doc.type(), doc.id(), create.uid())
+        final VersionConflictEngineException versionConflictEngineException = expectThrows(VersionConflictEngineException.class,
+                    () -> engine.get(new Engine.Get(true, false, doc.type(), doc.id(), create.uid())
                 .setIfSeqNo(indexResult.getSeqNo() + 1).setIfPrimaryTerm(primaryTerm.get() + 1),
             searcherFactory));
+        assertThat(versionConflictEngineException.getStackTrace(), emptyArray());
     }
 
     public void testVersioningNewIndex() throws IOException {
@@ -2028,6 +2031,7 @@ public class InternalEngineTests extends EngineTestCase {
                     assertThat(result.getVersion(), equalTo(lastOpVersion));
                     assertThat(result.getResultType(), equalTo(Engine.Result.Type.FAILURE));
                     assertThat(result.getFailure(), instanceOf(VersionConflictEngineException.class));
+                    assertThat(result.getFailure().getStackTrace(), emptyArray());
                 } else {
                     final Engine.IndexResult result;
                     if (versionedOp) {
@@ -2065,6 +2069,7 @@ public class InternalEngineTests extends EngineTestCase {
                     assertThat(result.getVersion(), equalTo(lastOpVersion));
                     assertThat(result.getResultType(), equalTo(Engine.Result.Type.FAILURE));
                     assertThat(result.getFailure(), instanceOf(VersionConflictEngineException.class));
+                    assertThat(result.getFailure().getStackTrace(), emptyArray());
                 } else {
                     final Engine.DeleteResult result;
                     long correctSeqNo = docDeleted ? UNASSIGNED_SEQ_NO : lastOpSeqNo;
@@ -2169,6 +2174,7 @@ public class InternalEngineTests extends EngineTestCase {
                     assertThat(result.getVersion(), equalTo(highestOpVersion));
                     assertThat(result.getResultType(), equalTo(Engine.Result.Type.FAILURE));
                     assertThat(result.getFailure(), instanceOf(VersionConflictEngineException.class));
+                    assertThat(result.getFailure().getStackTrace(), emptyArray());
                 }
             } else {
                 final Engine.Delete delete = (Engine.Delete) op;
@@ -2187,6 +2193,7 @@ public class InternalEngineTests extends EngineTestCase {
                     assertThat(result.getVersion(), equalTo(highestOpVersion));
                     assertThat(result.getResultType(), equalTo(Engine.Result.Type.FAILURE));
                     assertThat(result.getFailure(), instanceOf(VersionConflictEngineException.class));
+                    assertThat(result.getFailure().getStackTrace(), emptyArray());
                 }
             }
             if (randomBoolean()) {
@@ -4358,6 +4365,7 @@ public class InternalEngineTests extends EngineTestCase {
         assertThat(operation, result.getFailure(), Matchers.instanceOf(VersionConflictEngineException.class));
         VersionConflictEngineException exception = (VersionConflictEngineException) result.getFailure();
         assertThat(operation, exception.getMessage(), containsString("but no document was found"));
+        assertThat(exception.getStackTrace(), emptyArray());
     }
 
     /*