From 5ebcfa37304b24d19f666d76d5710944e1a31251 Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Mon, 16 Sep 2019 12:44:38 +0300 Subject: [PATCH 1/5] fix rollback bug Signed-off-by: Ratan Rai Sur --- plugin-api/build.gradle | 2 +- .../storage/KeyValueStorageFactory.java | 4 +- .../RocksDBKeyValuePrivacyStorageFactory.java | 3 +- .../RocksDBKeyValueStorageFactory.java | 128 +++++++++++------- .../RocksDBKeyValueStorageFactoryTest.java | 30 +++- 5 files changed, 114 insertions(+), 53 deletions(-) diff --git a/plugin-api/build.gradle b/plugin-api/build.gradle index d8640b0a367..f1f4982dffd 100644 --- a/plugin-api/build.gradle +++ b/plugin-api/build.gradle @@ -56,7 +56,7 @@ Calculated : ${currentHash} tasks.register('checkAPIChanges', FileStateChecker) { description = "Checks that the API for the Plugin-API project does not change without deliberate thought" files = sourceSets.main.allJava.files - knownHash = 'Qnc8VXZ1+kdpAlp2m+f5Kuxb1rwoxD74iBOGFpdDQ8Y=' + knownHash = 'Mmjk754aKE/NTT7s1Y2RSLrIYzmM43Mejs0STZN+4ZE=' } check.dependsOn('checkAPIChanges') diff --git a/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/storage/KeyValueStorageFactory.java b/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/storage/KeyValueStorageFactory.java index c454f114a14..69074216514 100644 --- a/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/storage/KeyValueStorageFactory.java +++ b/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/storage/KeyValueStorageFactory.java @@ -17,9 +17,11 @@ import org.hyperledger.besu.plugin.services.MetricsSystem; import org.hyperledger.besu.plugin.services.exception.StorageException; +import java.io.Closeable; + /** Factory for creating key-value storage instances. */ @Unstable -public interface KeyValueStorageFactory { +public interface KeyValueStorageFactory extends Closeable { /** * Retrieves the identity of the key-value storage factory. diff --git a/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValuePrivacyStorageFactory.java b/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValuePrivacyStorageFactory.java index c911c9b6575..eb266603213 100644 --- a/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValuePrivacyStorageFactory.java +++ b/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValuePrivacyStorageFactory.java @@ -37,8 +37,7 @@ public String getName() { return "rocksdb-privacy"; } - @Override protected Path storagePath(final BesuConfiguration commonConfiguration) { - return super.storagePath(commonConfiguration).resolve(PRIVATE_DATABASE_PATH); + return commonConfiguration.getStoragePath().resolve(PRIVATE_DATABASE_PATH); } } diff --git a/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactory.java b/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactory.java index f838fea63bb..7834e476702 100644 --- a/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactory.java +++ b/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactory.java @@ -12,8 +12,10 @@ */ package org.hyperledger.besu.plugin.services.storage.rocksdb; -import org.hyperledger.besu.plugin.services.BesuConfiguration; +import static com.google.common.base.Preconditions.checkNotNull; + import org.hyperledger.besu.plugin.services.MetricsSystem; +import org.hyperledger.besu.plugin.services.PantheonConfiguration; import org.hyperledger.besu.plugin.services.exception.StorageException; import org.hyperledger.besu.plugin.services.storage.KeyValueStorage; import org.hyperledger.besu.plugin.services.storage.KeyValueStorageFactory; @@ -40,22 +42,36 @@ public class RocksDBKeyValueStorageFactory implements KeyValueStorageFactory { private static final Logger LOG = LogManager.getLogger(); - private static final int DEFAULT_VERSION = 1; - private static final Set SUPPORTED_VERSION = Set.of(0, 1); + private final int DEFAULT_VERSION; + private static final Set SUPPORTED_VERSIONS = Set.of(0, 1); private static final String NAME = "rocksdb"; - private boolean isSegmentIsolationSupported; + private Integer databaseVersion; + private Boolean isSegmentIsolationSupported; private SegmentedKeyValueStorage segmentedStorage; private KeyValueStorage unsegmentedStorage; + private RocksDBConfiguration rocksDBConfiguration; private final Supplier configuration; private final List segments; - public RocksDBKeyValueStorageFactory( + RocksDBKeyValueStorageFactory( final Supplier configuration, - final List segments) { + final List segments, + final int DEFAULT_VERSION) { this.configuration = configuration; this.segments = segments; + this.DEFAULT_VERSION = DEFAULT_VERSION; + } + + public RocksDBKeyValueStorageFactory( + final Supplier configuration, + final List segments) { + this( + configuration, + segments, + /** Source of truth for the default database version. */ + 1); } @Override @@ -71,62 +87,61 @@ public KeyValueStorage create( throws StorageException { if (requiresInit()) { - init(commonConfiguration, metricsSystem); + init(commonConfiguration); } - return isSegmentIsolationSupported - ? new SegmentedKeyValueStorageAdapter<>(segment, segmentedStorage) - : unsegmentedStorage; - } - - @Override - public boolean isSegmentIsolationSupported() { - return isSegmentIsolationSupported; - } - - public void close() throws IOException { - if (segmentedStorage != null) { - segmentedStorage.close(); + // It's probably a good idea for the creation logic to be entirely dependent on the database + // version. Introducing intermediate booleans that represent database properties and dispatching + // creation logic based on them is error prone. + switch (databaseVersion) { + case 0: + { + segmentedStorage = null; + if (unsegmentedStorage == null) { + unsegmentedStorage = new RocksDBKeyValueStorage(rocksDBConfiguration, metricsSystem); + } + return unsegmentedStorage; + } + case 1: + { + unsegmentedStorage = null; + if (segmentedStorage == null) { + segmentedStorage = + new RocksDBColumnarKeyValueStorage(rocksDBConfiguration, segments, metricsSystem); + } + return new SegmentedKeyValueStorageAdapter<>(segment, segmentedStorage); + } + default: + { + throw new IllegalStateException( + String.format( + "Developer error: A supported database version (%d) was detected but there is no associated creation logic.", + databaseVersion)); + } } - if (unsegmentedStorage != null) { - unsegmentedStorage.close(); - } - } - - protected Path storagePath(final BesuConfiguration commonConfiguration) { - return commonConfiguration.getStoragePath(); - } - - private boolean requiresInit() { - return segmentedStorage == null && unsegmentedStorage == null; } - private void init( - final BesuConfiguration commonConfiguration, final MetricsSystem metricsSystem) { + private void init(final PantheonConfiguration commonConfiguration) { try { - this.isSegmentIsolationSupported = databaseVersion(commonConfiguration) == DEFAULT_VERSION; + databaseVersion = readDatabaseVersion(commonConfiguration); } catch (final IOException e) { LOG.error("Failed to retrieve the RocksDB database meta version: {}", e.getMessage()); throw new StorageException(e.getMessage(), e); } - - final RocksDBConfiguration rocksDBConfiguration = + isSegmentIsolationSupported = databaseVersion >= 1; + rocksDBConfiguration = RocksDBConfigurationBuilder.from(configuration.get()) - .databaseDir(storagePath(commonConfiguration)) + .databaseDir(commonConfiguration.getStoragePath()) .build(); + } - if (isSegmentIsolationSupported) { - this.unsegmentedStorage = null; - this.segmentedStorage = - new RocksDBColumnarKeyValueStorage(rocksDBConfiguration, segments, metricsSystem); - } else { - this.unsegmentedStorage = new RocksDBKeyValueStorage(rocksDBConfiguration, metricsSystem); - this.segmentedStorage = null; - } + private boolean requiresInit() { + return segmentedStorage == null && unsegmentedStorage == null; } - private int databaseVersion(final BesuConfiguration commonConfiguration) throws IOException { - final Path databaseDir = storagePath(commonConfiguration); + private int readDatabaseVersion(final BesuConfiguration commonConfiguration) + throws IOException { + final Path databaseDir = commonConfiguration.getStoragePath(); final boolean databaseExists = databaseDir.resolve("IDENTITY").toFile().exists(); final int databaseVersion; if (databaseExists) { @@ -140,7 +155,7 @@ private int databaseVersion(final BesuConfiguration commonConfiguration) throws new DatabaseMetadata(databaseVersion).writeToDirectory(databaseDir); } - if (!SUPPORTED_VERSION.contains(databaseVersion)) { + if (!SUPPORTED_VERSIONS.contains(databaseVersion)) { final String message = "Unsupported RocksDB Metadata version of: " + databaseVersion; LOG.error(message); throw new StorageException(message); @@ -148,4 +163,21 @@ private int databaseVersion(final BesuConfiguration commonConfiguration) throws return databaseVersion; } + + @Override + public void close() throws IOException { + if (unsegmentedStorage != null) { + unsegmentedStorage.close(); + } + if (segmentedStorage != null) { + segmentedStorage.close(); + } + } + + @Override + public boolean isSegmentIsolationSupported() { + return checkNotNull( + isSegmentIsolationSupported, + "Whether segment isolation is supported will be determined during creation. Call a creation method first"); + } } diff --git a/plugins/rocksdb/src/test/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactoryTest.java b/plugins/rocksdb/src/test/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactoryTest.java index 7a6f398c1b1..457357fd5e5 100644 --- a/plugins/rocksdb/src/test/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactoryTest.java +++ b/plugins/rocksdb/src/test/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactoryTest.java @@ -13,12 +13,13 @@ package org.hyperledger.besu.plugin.services.storage.rocksdb; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.when; import org.hyperledger.besu.metrics.ObservableMetricsSystem; import org.hyperledger.besu.metrics.noop.NoOpMetricsSystem; -import org.hyperledger.besu.plugin.services.BesuConfiguration; +import org.hyperledger.besu.plugin.services.PantheonConfiguration; import org.hyperledger.besu.plugin.services.exception.StorageException; import org.hyperledger.besu.plugin.services.storage.SegmentIdentifier; import org.hyperledger.besu.plugin.services.storage.rocksdb.configuration.DatabaseMetadata; @@ -96,6 +97,22 @@ public void shouldDetectCorrectVersionIfMetadataFileExists() throws Exception { assertThat(storageFactory.isSegmentIsolationSupported()).isTrue(); } + @Test + public void shouldDetectCorrectVersionInCaseOfRollback() throws Exception { + final Path tempDatabaseDir = temporaryFolder.newFolder().toPath().resolve("db"); + Files.createDirectories(tempDatabaseDir); + when(commonConfiguration.getStoragePath()).thenReturn(tempDatabaseDir); + final RocksDBKeyValueStorageFactory storageFactory = + new RocksDBKeyValueStorageFactory(() -> rocksDbConfiguration, segments, 1); + + storageFactory.create(segment, commonConfiguration, metricsSystem); + storageFactory.close(); + + final RocksDBKeyValueStorageFactory rolledbackStorageFactory = + new RocksDBKeyValueStorageFactory(() -> rocksDbConfiguration, segments, 0); + rolledbackStorageFactory.create(segment, commonConfiguration, metricsSystem); + } + @Test public void shouldThrowExceptionWhenVersionNumberIsInvalid() throws Exception { final Path tempDatabaseDir = temporaryFolder.newFolder().toPath().resolve("db"); @@ -111,6 +128,17 @@ public void shouldThrowExceptionWhenVersionNumberIsInvalid() throws Exception { .isInstanceOf(StorageException.class); } + @Test + public void shouldSetSegmentationFieldDuringCreation() throws Exception { + final Path tempDatabaseDir = temporaryFolder.newFolder().toPath().resolve("db"); + Files.createDirectories(tempDatabaseDir); + when(commonConfiguration.getStoragePath()).thenReturn(tempDatabaseDir); + final RocksDBKeyValueStorageFactory storageFactory = + new RocksDBKeyValueStorageFactory(() -> rocksDbConfiguration, segments); + storageFactory.create(segment, commonConfiguration, metricsSystem); + assertThatCode(storageFactory::isSegmentIsolationSupported).doesNotThrowAnyException(); + } + @Test public void shouldThrowExceptionWhenMetaDataFileIsCorrupted() throws Exception { final Path tempDatabaseDir = temporaryFolder.newFolder().toPath().resolve("db"); From 1f653d15092378d3311fc26ee75b1eed1bd86810 Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Mon, 16 Sep 2019 12:54:46 +0300 Subject: [PATCH 2/5] update hash Signed-off-by: Ratan Rai Sur --- plugin-api/build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin-api/build.gradle b/plugin-api/build.gradle index f1f4982dffd..82496dd7041 100644 --- a/plugin-api/build.gradle +++ b/plugin-api/build.gradle @@ -56,7 +56,7 @@ Calculated : ${currentHash} tasks.register('checkAPIChanges', FileStateChecker) { description = "Checks that the API for the Plugin-API project does not change without deliberate thought" files = sourceSets.main.allJava.files - knownHash = 'Mmjk754aKE/NTT7s1Y2RSLrIYzmM43Mejs0STZN+4ZE=' + knownHash = 'zCXMIJ79YKiQ7Ur251fchUNFzv3K4LMCBbl6TapJqHQ=' } check.dependsOn('checkAPIChanges') From a09836c14e8acce969a8ab85bda9f6e4a146d1b5 Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Mon, 16 Sep 2019 12:56:30 +0300 Subject: [PATCH 3/5] spotless Signed-off-by: Ratan Rai Sur --- .../storage/rocksdb/RocksDBKeyValueStorageFactory.java | 3 +-- .../storage/rocksdb/RocksDBKeyValueStorageFactoryTest.java | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactory.java b/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactory.java index 7834e476702..ec028e8f395 100644 --- a/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactory.java +++ b/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactory.java @@ -139,8 +139,7 @@ private boolean requiresInit() { return segmentedStorage == null && unsegmentedStorage == null; } - private int readDatabaseVersion(final BesuConfiguration commonConfiguration) - throws IOException { + private int readDatabaseVersion(final BesuConfiguration commonConfiguration) throws IOException { final Path databaseDir = commonConfiguration.getStoragePath(); final boolean databaseExists = databaseDir.resolve("IDENTITY").toFile().exists(); final int databaseVersion; diff --git a/plugins/rocksdb/src/test/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactoryTest.java b/plugins/rocksdb/src/test/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactoryTest.java index 457357fd5e5..358fe3c11c5 100644 --- a/plugins/rocksdb/src/test/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactoryTest.java +++ b/plugins/rocksdb/src/test/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactoryTest.java @@ -19,7 +19,6 @@ import org.hyperledger.besu.metrics.ObservableMetricsSystem; import org.hyperledger.besu.metrics.noop.NoOpMetricsSystem; -import org.hyperledger.besu.plugin.services.PantheonConfiguration; import org.hyperledger.besu.plugin.services.exception.StorageException; import org.hyperledger.besu.plugin.services.storage.SegmentIdentifier; import org.hyperledger.besu.plugin.services.storage.rocksdb.configuration.DatabaseMetadata; From 55e37473121169841ae3b2609ada6d956bba4577 Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Mon, 16 Sep 2019 13:02:42 +0300 Subject: [PATCH 4/5] fix rebase mistakes Signed-off-by: Ratan Rai Sur --- .../storage/rocksdb/RocksDBKeyValueStorageFactory.java | 4 ++-- .../storage/rocksdb/RocksDBKeyValueStorageFactoryTest.java | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactory.java b/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactory.java index ec028e8f395..1ebc2d27c4c 100644 --- a/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactory.java +++ b/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactory.java @@ -14,8 +14,8 @@ import static com.google.common.base.Preconditions.checkNotNull; +import org.hyperledger.besu.plugin.services.BesuConfiguration; import org.hyperledger.besu.plugin.services.MetricsSystem; -import org.hyperledger.besu.plugin.services.PantheonConfiguration; import org.hyperledger.besu.plugin.services.exception.StorageException; import org.hyperledger.besu.plugin.services.storage.KeyValueStorage; import org.hyperledger.besu.plugin.services.storage.KeyValueStorageFactory; @@ -121,7 +121,7 @@ public KeyValueStorage create( } } - private void init(final PantheonConfiguration commonConfiguration) { + private void init(final BesuConfiguration commonConfiguration) { try { databaseVersion = readDatabaseVersion(commonConfiguration); } catch (final IOException e) { diff --git a/plugins/rocksdb/src/test/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactoryTest.java b/plugins/rocksdb/src/test/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactoryTest.java index 358fe3c11c5..d4f12977492 100644 --- a/plugins/rocksdb/src/test/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactoryTest.java +++ b/plugins/rocksdb/src/test/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactoryTest.java @@ -19,6 +19,7 @@ import org.hyperledger.besu.metrics.ObservableMetricsSystem; import org.hyperledger.besu.metrics.noop.NoOpMetricsSystem; +import org.hyperledger.besu.plugin.services.BesuConfiguration; import org.hyperledger.besu.plugin.services.exception.StorageException; import org.hyperledger.besu.plugin.services.storage.SegmentIdentifier; import org.hyperledger.besu.plugin.services.storage.rocksdb.configuration.DatabaseMetadata; From 6e5c662e45da5ca11b839ee4e90328df66b5ae51 Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Tue, 17 Sep 2019 12:26:49 +0300 Subject: [PATCH 5/5] get acceptance tests passing Co-Authored-By: CJ Hare Signed-off-by: Ratan Rai Sur --- .../rocksdb/RocksDBKeyValuePrivacyStorageFactory.java | 4 ++-- .../storage/rocksdb/RocksDBKeyValueStorageFactory.java | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValuePrivacyStorageFactory.java b/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValuePrivacyStorageFactory.java index eb266603213..57cddc7fc68 100644 --- a/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValuePrivacyStorageFactory.java +++ b/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValuePrivacyStorageFactory.java @@ -36,8 +36,8 @@ public RocksDBKeyValuePrivacyStorageFactory( public String getName() { return "rocksdb-privacy"; } - +@Override protected Path storagePath(final BesuConfiguration commonConfiguration) { - return commonConfiguration.getStoragePath().resolve(PRIVATE_DATABASE_PATH); + return super.storagePath(commonConfiguration).resolve(PRIVATE_DATABASE_PATH); } } diff --git a/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactory.java b/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactory.java index 1ebc2d27c4c..a7343a45742 100644 --- a/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactory.java +++ b/plugins/rocksdb/src/main/java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactory.java @@ -120,7 +120,9 @@ public KeyValueStorage create( } } } - + protected Path storagePath(final BesuConfiguration commonConfiguration) { + return commonConfiguration.getStoragePath(); + } private void init(final BesuConfiguration commonConfiguration) { try { databaseVersion = readDatabaseVersion(commonConfiguration); @@ -131,7 +133,7 @@ private void init(final BesuConfiguration commonConfiguration) { isSegmentIsolationSupported = databaseVersion >= 1; rocksDBConfiguration = RocksDBConfigurationBuilder.from(configuration.get()) - .databaseDir(commonConfiguration.getStoragePath()) + .databaseDir(storagePath(commonConfiguration)) .build(); }