From c68ca5e399c46219deafa87809f7a462b40c9f32 Mon Sep 17 00:00:00 2001 From: Andrey Ershov Date: Thu, 29 Nov 2018 13:34:34 +0100 Subject: [PATCH 1/2] Zen2 storage layer WriteStateException propagation --- .../gateway/GatewayMetaState.java | 8 +-- .../gateway/WriteStateException.java | 13 +++++ .../gateway/WriteStateExceptionTests.java | 52 +++++++++++++++++++ 3 files changed, 69 insertions(+), 4 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/gateway/WriteStateExceptionTests.java diff --git a/server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java b/server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java index fecbc51ac6bc8..59e2ce9212da7 100644 --- a/server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java +++ b/server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java @@ -208,8 +208,8 @@ public void setCurrentTerm(long currentTerm) { metaStateService.writeManifestAndCleanup("current term changed", manifest); previousManifest = manifest; } catch (WriteStateException e) { - logger.warn("Exception occurred when setting current term", e); - //TODO re-throw exception + logger.error("Failed to set current term to {}", currentTerm); + e.rethrowAsErrorOrUncheckedException("setCurrentTerm failed"); } } @@ -221,8 +221,8 @@ public void setLastAcceptedState(ClusterState clusterState) { incrementalWrite = previousClusterState.term() == clusterState.term(); updateClusterState(clusterState, previousClusterState); } catch (WriteStateException e) { - logger.warn("Exception occurred when setting last accepted state", e); - //TODO re-throw exception + logger.error("Failed to set last accepted state with version {}", clusterState.version()); + e.rethrowAsErrorOrUncheckedException("setLastAcceptedState failed"); } } diff --git a/server/src/main/java/org/elasticsearch/gateway/WriteStateException.java b/server/src/main/java/org/elasticsearch/gateway/WriteStateException.java index aeb9bffc6711d..1963c547115bd 100644 --- a/server/src/main/java/org/elasticsearch/gateway/WriteStateException.java +++ b/server/src/main/java/org/elasticsearch/gateway/WriteStateException.java @@ -18,7 +18,9 @@ */ package org.elasticsearch.gateway; +import java.io.IOError; import java.io.IOException; +import java.io.UncheckedIOException; /** * This exception is thrown when there is a problem of writing state to disk. @@ -38,4 +40,15 @@ public class WriteStateException extends IOException { public boolean isDirty() { return dirty; } + + public void rethrowAsErrorOrUncheckedException(String msg) { + if (isDirty()) { + // IOError is the best thing we have in java library to indicate that serious IO problem has occurred. + // IOError will be caught by ElasticsearchUncaughtExceptionHandler and JVM will be halted. + // Sadly, it has no constructor that accepts error message, so we first wrap WriteStateException with IOException. + throw new IOError(new IOException(msg + ", storage is dirty", this)); + } else { + throw new UncheckedIOException(msg, this); + } + } } diff --git a/server/src/test/java/org/elasticsearch/gateway/WriteStateExceptionTests.java b/server/src/test/java/org/elasticsearch/gateway/WriteStateExceptionTests.java new file mode 100644 index 0000000000000..f77a165ce61e1 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/gateway/WriteStateExceptionTests.java @@ -0,0 +1,52 @@ +/* + * 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.gateway; + +import org.elasticsearch.test.ESTestCase; + +import java.io.IOError; +import java.io.UncheckedIOException; + +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; + +public class WriteStateExceptionTests extends ESTestCase { + + public void testDirtyFlag() { + boolean dirty = randomBoolean(); + WriteStateException ex = new WriteStateException(dirty, "test", null); + assertThat(ex.isDirty(), equalTo(dirty)); + } + + public void testNonDirtyRethrow() { + WriteStateException ex = new WriteStateException(false, "test", null); + UncheckedIOException ex2 = expectThrows(UncheckedIOException.class, () -> ex.rethrowAsErrorOrUncheckedException("test message")); + assertThat(ex2.getCause(), instanceOf(WriteStateException.class)); + assertThat(ex2.getMessage(), equalTo("test message")); + } + + public void testDirtyRethrow() { + WriteStateException ex = new WriteStateException(true, "test", null); + IOError err = expectThrows(IOError.class, () -> ex.rethrowAsErrorOrUncheckedException("test message")); + assertThat(err.getCause().getCause(), instanceOf(WriteStateException.class)); + assertThat(err.getMessage(), containsString("test message, storage is dirty")); + } +} From 4094b916958672d3c8e996e84d166ea26927ecdc Mon Sep 17 00:00:00 2001 From: Andrey Ershov Date: Thu, 6 Dec 2018 11:41:21 +0100 Subject: [PATCH 2/2] Fix code review issues --- .../org/elasticsearch/gateway/GatewayMetaState.java | 9 +++++---- .../elasticsearch/gateway/WriteStateException.java | 13 +++++++------ .../gateway/WriteStateExceptionTests.java | 9 +++------ 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java b/server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java index 23ac5e68d23d2..c5bfa9224900a 100644 --- a/server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java +++ b/server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java @@ -22,6 +22,7 @@ import com.carrotsearch.hppc.cursors.ObjectObjectCursor; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.message.ParameterizedMessage; import org.elasticsearch.Version; import org.elasticsearch.cluster.ClusterChangedEvent; import org.elasticsearch.cluster.ClusterName; @@ -235,8 +236,8 @@ public void setCurrentTerm(long currentTerm) { try { innerSetCurrentTerm(currentTerm); } catch (WriteStateException e) { - logger.error("Failed to set current term to {}", currentTerm); - e.rethrowAsErrorOrUncheckedException("setCurrentTerm failed"); + logger.error(new ParameterizedMessage("Failed to set current term to {}", currentTerm), e); + e.rethrowAsErrorOrUncheckedException(); } } @@ -253,8 +254,8 @@ public void setLastAcceptedState(ClusterState clusterState) { incrementalWrite = previousClusterState.term() == clusterState.term(); updateClusterState(clusterState, previousClusterState); } catch (WriteStateException e) { - logger.error("Failed to set last accepted state with version {}", clusterState.version()); - e.rethrowAsErrorOrUncheckedException("setLastAcceptedState failed"); + logger.error(new ParameterizedMessage("Failed to set last accepted state with version {}", clusterState.version()), e); + e.rethrowAsErrorOrUncheckedException(); } } diff --git a/server/src/main/java/org/elasticsearch/gateway/WriteStateException.java b/server/src/main/java/org/elasticsearch/gateway/WriteStateException.java index 1963c547115bd..29699be2c16a0 100644 --- a/server/src/main/java/org/elasticsearch/gateway/WriteStateException.java +++ b/server/src/main/java/org/elasticsearch/gateway/WriteStateException.java @@ -41,14 +41,15 @@ public boolean isDirty() { return dirty; } - public void rethrowAsErrorOrUncheckedException(String msg) { + /** + * Rethrows this {@link WriteStateException} as {@link IOError} if dirty flag is set, which will lead to JVM shutdown. + * If dirty flag is not set, this exception is wrapped into {@link UncheckedIOException}. + */ + public void rethrowAsErrorOrUncheckedException() { if (isDirty()) { - // IOError is the best thing we have in java library to indicate that serious IO problem has occurred. - // IOError will be caught by ElasticsearchUncaughtExceptionHandler and JVM will be halted. - // Sadly, it has no constructor that accepts error message, so we first wrap WriteStateException with IOException. - throw new IOError(new IOException(msg + ", storage is dirty", this)); + throw new IOError(this); } else { - throw new UncheckedIOException(msg, this); + throw new UncheckedIOException(this); } } } diff --git a/server/src/test/java/org/elasticsearch/gateway/WriteStateExceptionTests.java b/server/src/test/java/org/elasticsearch/gateway/WriteStateExceptionTests.java index f77a165ce61e1..cc6a7950d8920 100644 --- a/server/src/test/java/org/elasticsearch/gateway/WriteStateExceptionTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/WriteStateExceptionTests.java @@ -24,7 +24,6 @@ import java.io.IOError; import java.io.UncheckedIOException; -import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; @@ -38,15 +37,13 @@ public void testDirtyFlag() { public void testNonDirtyRethrow() { WriteStateException ex = new WriteStateException(false, "test", null); - UncheckedIOException ex2 = expectThrows(UncheckedIOException.class, () -> ex.rethrowAsErrorOrUncheckedException("test message")); + UncheckedIOException ex2 = expectThrows(UncheckedIOException.class, () -> ex.rethrowAsErrorOrUncheckedException()); assertThat(ex2.getCause(), instanceOf(WriteStateException.class)); - assertThat(ex2.getMessage(), equalTo("test message")); } public void testDirtyRethrow() { WriteStateException ex = new WriteStateException(true, "test", null); - IOError err = expectThrows(IOError.class, () -> ex.rethrowAsErrorOrUncheckedException("test message")); - assertThat(err.getCause().getCause(), instanceOf(WriteStateException.class)); - assertThat(err.getMessage(), containsString("test message, storage is dirty")); + IOError err = expectThrows(IOError.class, () -> ex.rethrowAsErrorOrUncheckedException()); + assertThat(err.getCause(), instanceOf(WriteStateException.class)); } }