From 82f30fbc1c510078888fbd89e27d8191bcd12a0f Mon Sep 17 00:00:00 2001 From: Radek Hubner Date: Thu, 13 Jun 2024 08:36:25 +0200 Subject: [PATCH 1/3] Throw exception for multiGet operation on corrupted database. --- java/CMakeLists.txt | 2 + java/rocksjni/jni_multiget_helpers.cc | 3 + .../org/rocksdb/MultiGetCorruptionTest.java | 73 +++++++++++++++++++ 3 files changed, 78 insertions(+) create mode 100644 java/src/test/java/org/rocksdb/MultiGetCorruptionTest.java diff --git a/java/CMakeLists.txt b/java/CMakeLists.txt index a60847ead37..d8fee41fce3 100644 --- a/java/CMakeLists.txt +++ b/java/CMakeLists.txt @@ -359,6 +359,7 @@ set(JAVA_TEST_CLASSES src/test/java/org/rocksdb/MergeVariantsTest.java src/test/java/org/rocksdb/MixedOptionsTest.java src/test/java/org/rocksdb/MultiColumnRegressionTest.java + src/test/java/org/rocksdb/MultiGetCorruptionTest.java src/test/java/org/rocksdb/MultiGetManyKeysTest.java src/test/java/org/rocksdb/MultiGetTest.java src/test/java/org/rocksdb/MutableColumnFamilyOptionsTest.java @@ -477,6 +478,7 @@ set(JAVA_TEST_RUNNING_CLASSES org.rocksdb.MergeVariantsTest org.rocksdb.MixedOptionsTest org.rocksdb.MultiColumnRegressionTest + org.rocksdb.MultiGetCorruptionTest org.rocksdb.MultiGetManyKeysTest org.rocksdb.MultiGetTest org.rocksdb.MutableColumnFamilyOptionsTest diff --git a/java/rocksjni/jni_multiget_helpers.cc b/java/rocksjni/jni_multiget_helpers.cc index 99994ac7cce..c6c60c341e6 100644 --- a/java/rocksjni/jni_multiget_helpers.cc +++ b/java/rocksjni/jni_multiget_helpers.cc @@ -165,6 +165,9 @@ jobjectArray MultiGetJNIValues::byteArrays( // retain this behaviour. To change it, we need to do the following: // ROCKSDB_NAMESPACE::RocksDBExceptionJni::ThrowNew(env, s[i]); // return nullptr; + } else if (s[i].code() == ROCKSDB_NAMESPACE::Status::Code::kCorruption) { + ROCKSDB_NAMESPACE::RocksDBExceptionJni::ThrowNew(env, s[i]); + return nullptr; } } return jresults; diff --git a/java/src/test/java/org/rocksdb/MultiGetCorruptionTest.java b/java/src/test/java/org/rocksdb/MultiGetCorruptionTest.java new file mode 100644 index 00000000000..6755df5e71b --- /dev/null +++ b/java/src/test/java/org/rocksdb/MultiGetCorruptionTest.java @@ -0,0 +1,73 @@ +package org.rocksdb; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.assertj.core.api.Assertions.assertThat; + +import java.io.*; +import java.util.ArrayList; +import java.util.List; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; +import org.junit.rules.TemporaryFolder; + +public class MultiGetCorruptionTest { + private static final byte[] KEY = "key".getBytes(UTF_8); + private static final byte[] VALUE; + + static { + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < 100; i++) { + sb.append("value" + i + "\n"); + } + VALUE = sb.toString().getBytes(UTF_8); + } + + @ClassRule + public static final RocksNativeLibraryResource ROCKS_NATIVE_LIBRARY_RESOURCE = + new RocksNativeLibraryResource(); + + @Rule public TemporaryFolder dbFolder = new TemporaryFolder(); + + @Rule public ExpectedException exception = ExpectedException.none(); + + @Test + public void getKeyException() throws RocksDBException, IOException { + createCorruptedDatabase(); + try (Options options = new Options().setCreateIfMissing(true).setParanoidChecks(true); + RocksDB db = RocksDB.openReadOnly(options, dbFolder.getRoot().getAbsolutePath())) { + exception.expect(RocksDBException.class); // We need to be sure, exception is thrown only + db.get(KEY); // on GET operation. Require careful data corruption. + } + } + + @Test + public void multiGetKeyException() throws RocksDBException, IOException { + createCorruptedDatabase(); + try (Options options = new Options().setCreateIfMissing(true).setParanoidChecks(true); + RocksDB db = RocksDB.openReadOnly(options, dbFolder.getRoot().getAbsolutePath())) { + // exception.expect(RocksDBException.class); + List keys = new ArrayList<>(); + keys.add(KEY); + List result = db.multiGetAsList(keys); + assertThat(result).isNotNull(); + } + } + + private void createCorruptedDatabase() throws RocksDBException, IOException { + try (Options options = new Options().setCreateIfMissing(true).setParanoidChecks(true); + RocksDB db = RocksDB.open(options, dbFolder.getRoot().getAbsolutePath())) { + db.put(KEY, VALUE); + db.flush(new FlushOptions().setWaitForFlush(true)); + } + + File[] files = dbFolder.getRoot().listFiles((dir, name) -> name.endsWith("sst")); + assertThat(files).hasSize(1); + + try (RandomAccessFile file = new RandomAccessFile(files[0], "rw")) { + file.seek(30); + file.write("corrupted".getBytes(UTF_8)); + } + } +} From 1db094972e63fbeaba15e91b806d8c26575f50c2 Mon Sep 17 00:00:00 2001 From: Radek Hubner Date: Fri, 14 Jun 2024 13:37:37 +0200 Subject: [PATCH 2/3] Fix multiGet corruption test and implementation. --- java/rocksjni/jni_multiget_helpers.cc | 6 +++--- .../org/rocksdb/MultiGetCorruptionTest.java | 19 ++++++++++++++++--- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/java/rocksjni/jni_multiget_helpers.cc b/java/rocksjni/jni_multiget_helpers.cc index c6c60c341e6..2909170e92e 100644 --- a/java/rocksjni/jni_multiget_helpers.cc +++ b/java/rocksjni/jni_multiget_helpers.cc @@ -158,6 +158,9 @@ jobjectArray MultiGetJNIValues::byteArrays( } env->DeleteLocalRef(jentry_value); + } else if (s[i].code() == ROCKSDB_NAMESPACE::Status::Code::kCorruption) { + ROCKSDB_NAMESPACE::RocksDBExceptionJni::ThrowNew(env, s[i]); + return nullptr; } else if (s[i].code() != ROCKSDB_NAMESPACE::Status::Code::kNotFound) { // The only way to return an error for a single key is to exception the // entire multiGet() Previous behaviour was to return a nullptr value for @@ -165,9 +168,6 @@ jobjectArray MultiGetJNIValues::byteArrays( // retain this behaviour. To change it, we need to do the following: // ROCKSDB_NAMESPACE::RocksDBExceptionJni::ThrowNew(env, s[i]); // return nullptr; - } else if (s[i].code() == ROCKSDB_NAMESPACE::Status::Code::kCorruption) { - ROCKSDB_NAMESPACE::RocksDBExceptionJni::ThrowNew(env, s[i]); - return nullptr; } } return jresults; diff --git a/java/src/test/java/org/rocksdb/MultiGetCorruptionTest.java b/java/src/test/java/org/rocksdb/MultiGetCorruptionTest.java index 6755df5e71b..bc8159dd428 100644 --- a/java/src/test/java/org/rocksdb/MultiGetCorruptionTest.java +++ b/java/src/test/java/org/rocksdb/MultiGetCorruptionTest.java @@ -6,6 +6,8 @@ import java.io.*; import java.util.ArrayList; import java.util.List; +import org.hamcrest.Description; +import org.hamcrest.TypeSafeMatcher; import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; @@ -47,11 +49,22 @@ public void multiGetKeyException() throws RocksDBException, IOException { createCorruptedDatabase(); try (Options options = new Options().setCreateIfMissing(true).setParanoidChecks(true); RocksDB db = RocksDB.openReadOnly(options, dbFolder.getRoot().getAbsolutePath())) { - // exception.expect(RocksDBException.class); + exception.expect(RocksDBException.class); + exception.expect(new TypeSafeMatcher() { + @Override + protected boolean matchesSafely(RocksDBException e) { + return e.getStatus().getCode() == Status.Code.Corruption; + } + + @Override + public void describeTo(Description description) { + description.appendText("Status.Code is not equal to Corruption"); + } + }); + List keys = new ArrayList<>(); keys.add(KEY); - List result = db.multiGetAsList(keys); - assertThat(result).isNotNull(); + db.multiGetAsList(keys); } } From 99d632ebbb0d04e8f6e699e62ec11678ba510c86 Mon Sep 17 00:00:00 2001 From: Alan Paxton Date: Tue, 9 Jul 2024 17:43:25 +0100 Subject: [PATCH 3/3] Polish the test message - sense was inverted when it failed --- .../org/rocksdb/MultiGetCorruptionTest.java | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/java/src/test/java/org/rocksdb/MultiGetCorruptionTest.java b/java/src/test/java/org/rocksdb/MultiGetCorruptionTest.java index bc8159dd428..0a47550dcab 100644 --- a/java/src/test/java/org/rocksdb/MultiGetCorruptionTest.java +++ b/java/src/test/java/org/rocksdb/MultiGetCorruptionTest.java @@ -3,11 +3,13 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.assertj.core.api.Assertions.assertThat; -import java.io.*; +import java.io.File; +import java.io.IOException; +import java.io.RandomAccessFile; import java.util.ArrayList; import java.util.List; +import org.hamcrest.CustomTypeSafeMatcher; import org.hamcrest.Description; -import org.hamcrest.TypeSafeMatcher; import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; @@ -50,15 +52,17 @@ public void multiGetKeyException() throws RocksDBException, IOException { try (Options options = new Options().setCreateIfMissing(true).setParanoidChecks(true); RocksDB db = RocksDB.openReadOnly(options, dbFolder.getRoot().getAbsolutePath())) { exception.expect(RocksDBException.class); - exception.expect(new TypeSafeMatcher() { + exception.expect(new CustomTypeSafeMatcher( + "Status.Code equal to Corruption") { @Override protected boolean matchesSafely(RocksDBException e) { return e.getStatus().getCode() == Status.Code.Corruption; } @Override - public void describeTo(Description description) { - description.appendText("Status.Code is not equal to Corruption"); + protected void describeMismatchSafely(RocksDBException e, Description mismatchDescription) { + mismatchDescription.appendText( + "was " + e.getStatus().getCodeString() + " " + e.getMessage()); } }); @@ -72,7 +76,9 @@ private void createCorruptedDatabase() throws RocksDBException, IOException { try (Options options = new Options().setCreateIfMissing(true).setParanoidChecks(true); RocksDB db = RocksDB.open(options, dbFolder.getRoot().getAbsolutePath())) { db.put(KEY, VALUE); - db.flush(new FlushOptions().setWaitForFlush(true)); + try (FlushOptions flushOptions = new FlushOptions().setWaitForFlush(true)) { + db.flush(flushOptions); + } } File[] files = dbFolder.getRoot().listFiles((dir, name) -> name.endsWith("sst"));