From db78e746d7e2fb9a2aa4c10e1322c6c5033f006b Mon Sep 17 00:00:00 2001 From: Stefan Miklosovic Date: Mon, 17 Apr 2023 12:20:21 +0200 Subject: [PATCH] Do not remove SSTables when cause of FSReadError is OutOfMemoryError while using best_effort disk failure policy patch by Stefan Miklosovic; reviewed by Brandon Williams and Maxwell Guo for CASSANDRA-18336 --- CHANGES.txt | 1 + .../service/DefaultFSErrorHandler.java | 22 +++++- .../service/DiskFailurePolicyTest.java | 76 +++++++++++++------ 3 files changed, 74 insertions(+), 25 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index a46d8a6c04..0834f18117 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 3.0.29 + * Do not remove SSTables when cause of FSReadError is OutOfMemoryError while using best_effort disk failure policy (CASSANDRA-18336) * Do not remove truncated_at entry in system.local while dropping an index (CASSANDRA-18105) * Save host id to system.local and flush immediately after startup (CASSANDRA-18153) * Fix RepairJob unnecessarily reporting cancellation error (CASSANDRA-17701) diff --git a/src/java/org/apache/cassandra/service/DefaultFSErrorHandler.java b/src/java/org/apache/cassandra/service/DefaultFSErrorHandler.java index 00029e3f40..2cf75d22e9 100644 --- a/src/java/org/apache/cassandra/service/DefaultFSErrorHandler.java +++ b/src/java/org/apache/cassandra/service/DefaultFSErrorHandler.java @@ -19,7 +19,9 @@ package org.apache.cassandra.service; import java.io.File; +import java.util.Set; +import com.google.common.collect.ImmutableSet; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -36,6 +38,8 @@ public class DefaultFSErrorHandler implements FSErrorHandler { private static final Logger logger = LoggerFactory.getLogger(DefaultFSErrorHandler.class); + private static final Set> exceptionsSkippingDataRemoval = ImmutableSet.of(OutOfMemoryError.class); + @Override public void handleCorruptSSTable(CorruptSSTableException e) { @@ -71,7 +75,7 @@ public class DefaultFSErrorHandler implements FSErrorHandler case best_effort: // for both read and write errors mark the path as unwritable. DisallowedDirectories.maybeMarkUnwritable(e.path); - if (e instanceof FSReadError) + if (e instanceof FSReadError && shouldMaybeRemoveData(e)) { File directory = DisallowedDirectories.maybeMarkUnreadable(e.path); if (directory != null) @@ -86,6 +90,22 @@ public class DefaultFSErrorHandler implements FSErrorHandler } } + private boolean shouldMaybeRemoveData(Throwable error) + { + for (Throwable t = error; t != null; t = t.getCause()) + { + for (Class c : exceptionsSkippingDataRemoval) + if (c.isAssignableFrom(t.getClass())) + return false; + for (Throwable s : t.getSuppressed()) + for (Class c : exceptionsSkippingDataRemoval) + if (c.isAssignableFrom(s.getClass())) + return false; + } + + return true; + } + private static void handleStartupFSError(Throwable t) { switch (DatabaseDescriptor.getDiskFailurePolicy()) diff --git a/test/unit/org/apache/cassandra/service/DiskFailurePolicyTest.java b/test/unit/org/apache/cassandra/service/DiskFailurePolicyTest.java index 90e85e9cc0..c3b4dfa57d 100644 --- a/test/unit/org/apache/cassandra/service/DiskFailurePolicyTest.java +++ b/test/unit/org/apache/cassandra/service/DiskFailurePolicyTest.java @@ -18,12 +18,12 @@ package org.apache.cassandra.service; +import java.io.File; import java.io.IOException; import java.util.Arrays; import java.util.Collection; import org.junit.After; -import org.junit.Assert; import org.junit.Before; import org.junit.BeforeClass; import org.junit.Test; @@ -31,9 +31,9 @@ import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import org.apache.cassandra.SchemaLoader; -import org.apache.cassandra.config.Config; import org.apache.cassandra.config.Config.DiskFailurePolicy; import org.apache.cassandra.config.DatabaseDescriptor; +import org.apache.cassandra.db.DisallowedDirectories; import org.apache.cassandra.exceptions.ConfigurationException; import org.apache.cassandra.gms.Gossiper; import org.apache.cassandra.io.FSReadError; @@ -42,6 +42,15 @@ import org.apache.cassandra.io.util.FileUtils; import org.apache.cassandra.utils.JVMStabilityInspector; import org.apache.cassandra.utils.KillerForTests; +import static org.apache.cassandra.config.Config.DiskFailurePolicy.best_effort; +import static org.apache.cassandra.config.Config.DiskFailurePolicy.die; +import static org.apache.cassandra.config.Config.DiskFailurePolicy.ignore; +import static org.apache.cassandra.config.Config.DiskFailurePolicy.stop; +import static org.apache.cassandra.config.Config.DiskFailurePolicy.stop_paranoid; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + @RunWith(Parameterized.class) public class DiskFailurePolicyTest { @@ -79,22 +88,24 @@ public class DiskFailurePolicyTest public static Collection generateData() { return Arrays.asList(new Object[][]{ - { Config.DiskFailurePolicy.die, true, new FSReadError(new IOException(), "blah"), false, true, true}, - { DiskFailurePolicy.ignore, true, new FSReadError(new IOException(), "blah"), true, false, false}, - { DiskFailurePolicy.stop, true, new FSReadError(new IOException(), "blah"), false, true, true}, - { DiskFailurePolicy.stop_paranoid, true, new FSReadError(new IOException(), "blah"), false, true, true}, - { Config.DiskFailurePolicy.die, true, new CorruptSSTableException(new IOException(), "blah"), false, true, true}, - { DiskFailurePolicy.ignore, true, new CorruptSSTableException(new IOException(), "blah"), true, false, false}, - { DiskFailurePolicy.stop, true, new CorruptSSTableException(new IOException(), "blah"), false, true, true}, - { DiskFailurePolicy.stop_paranoid, true, new CorruptSSTableException(new IOException(), "blah"), false, true, true}, - { Config.DiskFailurePolicy.die, false, new FSReadError(new IOException(), "blah"), false, true, false}, - { DiskFailurePolicy.ignore, false, new FSReadError(new IOException(), "blah"), true, false, false}, - { DiskFailurePolicy.stop, false, new FSReadError(new IOException(), "blah"), false, false, false}, - { DiskFailurePolicy.stop_paranoid, false, new FSReadError(new IOException(), "blah"), false, false, false}, - { Config.DiskFailurePolicy.die, false, new CorruptSSTableException(new IOException(), "blah"), false, true, false}, - { DiskFailurePolicy.ignore, false, new CorruptSSTableException(new IOException(), "blah"), true, false, false}, - { DiskFailurePolicy.stop, false, new CorruptSSTableException(new IOException(), "blah"), true, false, false}, - { DiskFailurePolicy.stop_paranoid, false, new CorruptSSTableException(new IOException(), "blah"), false, false, false} + { die, true, new FSReadError(new IOException(), "blah"), false, true, true}, + { ignore, true, new FSReadError(new IOException(), "blah"), true, false, false}, + { stop, true, new FSReadError(new IOException(), "blah"), false, true, true}, + { stop_paranoid, true, new FSReadError(new IOException(), "blah"), false, true, true}, + { die, true, new CorruptSSTableException(new IOException(), "blah"), false, true, true}, + { ignore, true, new CorruptSSTableException(new IOException(), "blah"), true, false, false}, + { stop, true, new CorruptSSTableException(new IOException(), "blah"), false, true, true}, + { stop_paranoid, true, new CorruptSSTableException(new IOException(), "blah"), false, true, true}, + { die, false, new FSReadError(new IOException(), "blah"), false, true, false}, + { ignore, false, new FSReadError(new IOException(), "blah"), true, false, false}, + { stop, false, new FSReadError(new IOException(), "blah"), false, false, false}, + { stop_paranoid, false, new FSReadError(new IOException(), "blah"), false, false, false}, + { die, false, new CorruptSSTableException(new IOException(), "blah"), false, true, false}, + { ignore, false, new CorruptSSTableException(new IOException(), "blah"), true, false, false}, + { stop, false, new CorruptSSTableException(new IOException(), "blah"), true, false, false}, + { stop_paranoid, false, new CorruptSSTableException(new IOException(), "blah"), false, false, false}, + { best_effort, false, new FSReadError(new IOException(new OutOfMemoryError("Java heap space test")), "best_effort_oom"), true, false, false}, + { best_effort, false, new FSReadError(new IOException(), "best_effort_io_exception"), true, false, false}, } ); } @@ -110,7 +121,7 @@ public class DiskFailurePolicyTest originalKiller = JVMStabilityInspector.replaceKiller(killerForTests); originalDiskFailurePolicy = DatabaseDescriptor.getDiskFailurePolicy(); StorageService.instance.startGossiping(); - Assert.assertTrue(Gossiper.instance.isEnabled()); + assertTrue(Gossiper.instance.isEnabled()); } @After @@ -124,12 +135,29 @@ public class DiskFailurePolicyTest public void testPolicies() { DatabaseDescriptor.setDiskFailurePolicy(testPolicy); - JVMStabilityInspector.inspectThrowable(t); - Assert.assertEquals(expectJVMKilled, killerForTests.wasKilled()); - Assert.assertEquals(expectJVMKilledQuiet, killerForTests.wasKilledQuietly()); - if (!expectJVMKilled) { + try + { + JVMStabilityInspector.inspectThrowable(t); + } + catch (OutOfMemoryError e) + { + if (!e.getMessage().equals("Java heap space test")) + throw e; + } + + if (testPolicy == best_effort && ((FSReadError) t).path.getName().equals("best_effort_io_exception")) + assertTrue(DisallowedDirectories.isUnreadable(new File("best_effort_io_exception"))); + + // when we have OOM, as cause, there is no reason to remove data + if (testPolicy == best_effort && ((FSReadError) t).path.getName().equals("best_effort_oom")) + assertFalse(DisallowedDirectories.isUnreadable(new File("best_effort_oom"))); + + assertEquals(expectJVMKilled, killerForTests.wasKilled()); + assertEquals(expectJVMKilledQuiet, killerForTests.wasKilledQuietly()); + if (!expectJVMKilled) + { // only verify gossip if JVM is not killed - Assert.assertEquals(expectGossipRunning, Gossiper.instance.isEnabled()); + assertEquals(expectGossipRunning, Gossiper.instance.isEnabled()); } } }