From 5daabfb44affa4cf521fe8dfc0258a2f713851ee Mon Sep 17 00:00:00 2001 From: Atanas Atanasov Date: Tue, 26 Nov 2024 14:57:41 +0200 Subject: [PATCH] WIP address some pr comments Signed-off-by: Atanas Atanasov --- .../block/common/utils/Preconditions.java | 2 +- server/docker/update-env.sh | 2 +- server/docs/configuration.md | 2 +- .../PersistenceInjectionModule.java | 32 +++++++++---------- .../storage/PersistenceStorageConfig.java | 8 ++--- .../storage/path/NoOpBlockPathResolver.java | 3 ++ .../storage/write/BlockAsFileWriter.java | 3 +- server/src/main/resources/app.properties | 2 +- .../PersistenceInjectionModuleTest.java | 2 +- .../storage/PersistenceStorageConfigTest.java | 8 ++--- .../storage/write/BlockAsDirWriterTest.java | 1 - server/src/test/resources/app.properties | 2 +- 12 files changed, 35 insertions(+), 32 deletions(-) diff --git a/common/src/main/java/com/hedera/block/common/utils/Preconditions.java b/common/src/main/java/com/hedera/block/common/utils/Preconditions.java index 17908df5..018c274d 100644 --- a/common/src/main/java/com/hedera/block/common/utils/Preconditions.java +++ b/common/src/main/java/com/hedera/block/common/utils/Preconditions.java @@ -124,7 +124,7 @@ public static long requirePositive(final long toCheck) { * positive */ public static long requirePositive(final long toCheck, final String errorMessage) { - if (0 >= toCheck) { + if (0L >= toCheck) { final String message = Objects.isNull(errorMessage) ? "The input long [%d] is required be positive.".formatted(toCheck) : errorMessage; diff --git a/server/docker/update-env.sh b/server/docker/update-env.sh index 5e4c1241..3b3deae2 100755 --- a/server/docker/update-env.sh +++ b/server/docker/update-env.sh @@ -28,7 +28,7 @@ fi if [ true = "$is_smoke_test" ]; then # add smoke test variables - echo "PERSISTENCE_STORAGE_TYPE=BLOCK_AS_DIR" >> .env #todo maybe in the future this needs to use BLOCK_AS_FILE? + echo "PERSISTENCE_STORAGE_TYPE=BLOCK_AS_LOCAL_DIRECTORY" >> .env #todo maybe in the future this needs to use BLOCK_AS_LOCAL_FILE? echo "MEDIATOR_RING_BUFFER_SIZE=1024" >> .env echo "NOTIFIER_RING_BUFFER_SIZE=1024" >> .env echo "JAVA_OPTS='-Xms4G -Xmx4G'" >> .env diff --git a/server/docs/configuration.md b/server/docs/configuration.md index e53a9743..342bc5af 100644 --- a/server/docs/configuration.md +++ b/server/docs/configuration.md @@ -12,7 +12,7 @@ defaults and can be left unchanged. It is recommended to browse the properties b | Environment Variable | Description | Default Value | |:---|:---|---:| | PERSISTENCE_STORAGE_ROOT_PATH | The root path for the storage, if not provided will attempt to create a `data` on the working dir of the app. | | -| PERSISTENCE_STORAGE_TYPE | Type of the persistence storage | BLOCK_AS_FILE | +| PERSISTENCE_STORAGE_TYPE | Type of the persistence storage | BLOCK_AS_LOCAL_FILE | | CONSUMER_TIMEOUT_THRESHOLD_MILLIS | Time to wait for subscribers before disconnecting in milliseconds | 1500 | | SERVICE_DELAY_MILLIS | Service shutdown delay in milliseconds | 500 | | MEDIATOR_RING_BUFFER_SIZE | Size of the ring buffer used by the mediator (must be a power of 2) | 67108864 | diff --git a/server/src/main/java/com/hedera/block/server/persistence/PersistenceInjectionModule.java b/server/src/main/java/com/hedera/block/server/persistence/PersistenceInjectionModule.java index 7096b825..191f12bd 100644 --- a/server/src/main/java/com/hedera/block/server/persistence/PersistenceInjectionModule.java +++ b/server/src/main/java/com/hedera/block/server/persistence/PersistenceInjectionModule.java @@ -68,7 +68,6 @@ static BlockWriter> providesBlockWriter( @NonNull final BlockNodeContext blockNodeContext, @NonNull final BlockRemover blockRemover, @NonNull final BlockPathResolver blockPathResolver) { - Objects.requireNonNull(blockNodeContext); Objects.requireNonNull(blockRemover); Objects.requireNonNull(blockPathResolver); final StorageType persistenceType = blockNodeContext @@ -77,13 +76,13 @@ static BlockWriter> providesBlockWriter( .type(); try { return switch (persistenceType) { - case BLOCK_AS_FILE -> BlockAsFileWriterBuilder.newBuilder( + case BLOCK_AS_LOCAL_FILE -> BlockAsFileWriterBuilder.newBuilder( blockNodeContext, blockRemover, blockPathResolver) .build(); - case BLOCK_AS_DIR -> BlockAsDirWriterBuilder.newBuilder( + case BLOCK_AS_LOCAL_DIRECTORY -> BlockAsDirWriterBuilder.newBuilder( blockNodeContext, blockRemover, blockPathResolver) .build(); - case NOOP -> new NoOpBlockWriter(); + case NO_OP -> new NoOpBlockWriter(); }; } catch (final IOException e) { // we cannot have checked exceptions with dagger @Provides @@ -101,11 +100,11 @@ static BlockWriter> providesBlockWriter( @Provides @Singleton static BlockReader providesBlockReader(@NonNull final PersistenceStorageConfig config) { - final StorageType persistenceType = Objects.requireNonNull(config).type(); + final StorageType persistenceType = config.type(); return switch (persistenceType) { - case BLOCK_AS_FILE -> BlockAsFileReaderBuilder.newBuilder().build(); - case BLOCK_AS_DIR -> BlockAsDirReaderBuilder.newBuilder(config).build(); - case NOOP -> new NoOpBlockReader(); + case BLOCK_AS_LOCAL_FILE -> BlockAsFileReaderBuilder.newBuilder().build(); + case BLOCK_AS_LOCAL_DIRECTORY -> BlockAsDirReaderBuilder.newBuilder(config).build(); + case NO_OP -> new NoOpBlockReader(); }; } @@ -121,11 +120,12 @@ static BlockReader providesBlockReader(@NonNull final Persistence @Singleton static BlockRemover providesBlockRemover( @NonNull final PersistenceStorageConfig config, @NonNull final BlockPathResolver blockPathResolver) { - final StorageType persistenceType = Objects.requireNonNull(config).type(); + Objects.requireNonNull(blockPathResolver); + final StorageType persistenceType = config.type(); return switch (persistenceType) { - case BLOCK_AS_FILE -> new BlockAsFileRemover(); - case BLOCK_AS_DIR -> new BlockAsDirRemover(blockPathResolver); - case NOOP -> new NoOpRemover(); + case BLOCK_AS_LOCAL_FILE -> new BlockAsFileRemover(); + case BLOCK_AS_LOCAL_DIRECTORY -> new BlockAsDirRemover(blockPathResolver); + case NO_OP -> new NoOpRemover(); }; } @@ -139,12 +139,12 @@ static BlockRemover providesBlockRemover( @Provides @Singleton static BlockPathResolver providesPathResolver(@NonNull final PersistenceStorageConfig config) { - final StorageType persistenceType = Objects.requireNonNull(config).type(); + final StorageType persistenceType = config.type(); final Path blockStorageRoot = Path.of(config.rootPath()); return switch (persistenceType) { - case BLOCK_AS_FILE -> new BlockAsFilePathResolver(blockStorageRoot); - case BLOCK_AS_DIR -> new BlockAsDirPathResolver(blockStorageRoot); - case NOOP -> new NoOpBlockPathResolver(); + case BLOCK_AS_LOCAL_FILE -> new BlockAsFilePathResolver(blockStorageRoot); + case BLOCK_AS_LOCAL_DIRECTORY -> new BlockAsDirPathResolver(blockStorageRoot); + case NO_OP -> new NoOpBlockPathResolver(); }; } diff --git a/server/src/main/java/com/hedera/block/server/persistence/storage/PersistenceStorageConfig.java b/server/src/main/java/com/hedera/block/server/persistence/storage/PersistenceStorageConfig.java index cb264852..fd28db0e 100644 --- a/server/src/main/java/com/hedera/block/server/persistence/storage/PersistenceStorageConfig.java +++ b/server/src/main/java/com/hedera/block/server/persistence/storage/PersistenceStorageConfig.java @@ -40,7 +40,7 @@ @ConfigData("persistence.storage") public record PersistenceStorageConfig( @ConfigProperty(defaultValue = "") String rootPath, - @ConfigProperty(defaultValue = "BLOCK_AS_FILE") StorageType type) { + @ConfigProperty(defaultValue = "BLOCK_AS_LOCAL_FILE") StorageType type) { private static final System.Logger LOGGER = System.getLogger(PersistenceStorageConfig.class.getName()); /** @@ -94,16 +94,16 @@ public enum StorageType { * This is also the default setting for the server if it is not * explicitly specified via an environment variable or app.properties. */ - BLOCK_AS_FILE, + BLOCK_AS_LOCAL_FILE, /** * This type of storage stores Blocks as directories with the Block * number being the directory number. Block Items are stored as files * within a given Block directory. Used primarily for testing purposes. */ - BLOCK_AS_DIR, + BLOCK_AS_LOCAL_DIRECTORY, /** * This type of storage does nothing. */ - NOOP + NO_OP } } diff --git a/server/src/main/java/com/hedera/block/server/persistence/storage/path/NoOpBlockPathResolver.java b/server/src/main/java/com/hedera/block/server/persistence/storage/path/NoOpBlockPathResolver.java index a71e7da8..57a97de5 100644 --- a/server/src/main/java/com/hedera/block/server/persistence/storage/path/NoOpBlockPathResolver.java +++ b/server/src/main/java/com/hedera/block/server/persistence/storage/path/NoOpBlockPathResolver.java @@ -29,5 +29,8 @@ public final class NoOpBlockPathResolver implements BlockPathResolver { @Override public Path resolvePathToBlock(final long blockNumber) { return null; + // todo should we return some path here so it is not null? + // reason would be to not have null pointer somewhere in our code + // if for whatever reason this no op impl needs to be used? } } diff --git a/server/src/main/java/com/hedera/block/server/persistence/storage/write/BlockAsFileWriter.java b/server/src/main/java/com/hedera/block/server/persistence/storage/write/BlockAsFileWriter.java index 2d14e781..7c7c6298 100644 --- a/server/src/main/java/com/hedera/block/server/persistence/storage/write/BlockAsFileWriter.java +++ b/server/src/main/java/com/hedera/block/server/persistence/storage/write/BlockAsFileWriter.java @@ -104,7 +104,8 @@ private List writeToFs(final BlockUnparsed blockToWrite) thro Files.createFile(blockToWritePathResolved); // todo maybe this is not the place to handle the exceptions, but maybe if we do a retry mechanism we - // must catch here + // must catch here. also should we repair permissions and use the remover to remove as a cleanup? + // do we have such cases? try (final FileOutputStream fos = new FileOutputStream(blockToWritePathResolved.toFile())) { BlockUnparsed.PROTOBUF.toBytes(blockToWrite).writeTo(fos); // todo what should be fallback logic if something goes wrong here? we attempt to resolve the path diff --git a/server/src/main/resources/app.properties b/server/src/main/resources/app.properties index 22e500f3..10b96628 100644 --- a/server/src/main/resources/app.properties +++ b/server/src/main/resources/app.properties @@ -11,4 +11,4 @@ prometheus.endpointPortNumber=9999 #Persistence Storage Config #persistence.storage.rootPath= -#persistence.storage.type=BLOCK_AS_DIR \ No newline at end of file +#persistence.storage.type=BLOCK_AS_LOCAL_DIRECTORY diff --git a/server/src/test/java/com/hedera/block/server/persistence/PersistenceInjectionModuleTest.java b/server/src/test/java/com/hedera/block/server/persistence/PersistenceInjectionModuleTest.java index 1facb4db..25dcd55c 100644 --- a/server/src/test/java/com/hedera/block/server/persistence/PersistenceInjectionModuleTest.java +++ b/server/src/test/java/com/hedera/block/server/persistence/PersistenceInjectionModuleTest.java @@ -92,7 +92,7 @@ void testProvidesBlockWriter_IOException() { final PersistenceStorageConfig persistenceStorageConfig = mock(PersistenceStorageConfig.class); when(persistenceStorageConfig.rootPath()).thenReturn("/invalid_path/:invalid_directory"); - when(persistenceStorageConfig.type()).thenReturn(StorageType.BLOCK_AS_DIR); + when(persistenceStorageConfig.type()).thenReturn(StorageType.BLOCK_AS_LOCAL_DIRECTORY); final Configuration configuration = mock(Configuration.class); when(blockNodeContext.configuration()).thenReturn(configuration); diff --git a/server/src/test/java/com/hedera/block/server/persistence/storage/PersistenceStorageConfigTest.java b/server/src/test/java/com/hedera/block/server/persistence/storage/PersistenceStorageConfigTest.java index b4ab66b6..47201b32 100644 --- a/server/src/test/java/com/hedera/block/server/persistence/storage/PersistenceStorageConfigTest.java +++ b/server/src/test/java/com/hedera/block/server/persistence/storage/PersistenceStorageConfigTest.java @@ -37,7 +37,7 @@ void testPersistenceStorageConfig_happyPath() throws IOException { Path testPath = Files.createTempDirectory(TEMP_DIR); PersistenceStorageConfig persistenceStorageConfig = - new PersistenceStorageConfig(testPath.toString(), StorageType.BLOCK_AS_FILE); + new PersistenceStorageConfig(testPath.toString(), StorageType.BLOCK_AS_LOCAL_FILE); assertEquals(testPath.toString(), persistenceStorageConfig.rootPath()); } @@ -49,7 +49,7 @@ void testPersistenceStorageConfig_emptyRootPath() throws IOException { deleteDirectory(Paths.get(expectedDefaultRootPath)); PersistenceStorageConfig persistenceStorageConfig = - new PersistenceStorageConfig(getAbsoluteFolder("data_empty"), StorageType.BLOCK_AS_FILE); + new PersistenceStorageConfig(getAbsoluteFolder("data_empty"), StorageType.BLOCK_AS_LOCAL_FILE); assertEquals(expectedDefaultRootPath, persistenceStorageConfig.rootPath()); } @@ -57,7 +57,7 @@ void testPersistenceStorageConfig_emptyRootPath() throws IOException { void persistenceStorageConfig_throwsExceptionForRelativePath() { IllegalArgumentException exception = assertThrows( IllegalArgumentException.class, - () -> new PersistenceStorageConfig("relative/path", StorageType.BLOCK_AS_FILE)); + () -> new PersistenceStorageConfig("relative/path", StorageType.BLOCK_AS_LOCAL_FILE)); assertEquals("relative/path Root path must be absolute", exception.getMessage()); } @@ -67,7 +67,7 @@ void persistenceStorageConfig_throwsRuntimeExceptionOnIOException() { RuntimeException exception = assertThrows( RuntimeException.class, - () -> new PersistenceStorageConfig(invalidPath.toString(), StorageType.BLOCK_AS_FILE)); + () -> new PersistenceStorageConfig(invalidPath.toString(), StorageType.BLOCK_AS_LOCAL_FILE)); assertInstanceOf(IOException.class, exception.getCause()); } diff --git a/server/src/test/java/com/hedera/block/server/persistence/storage/write/BlockAsDirWriterTest.java b/server/src/test/java/com/hedera/block/server/persistence/storage/write/BlockAsDirWriterTest.java index cab21be9..f4886b14 100644 --- a/server/src/test/java/com/hedera/block/server/persistence/storage/write/BlockAsDirWriterTest.java +++ b/server/src/test/java/com/hedera/block/server/persistence/storage/write/BlockAsDirWriterTest.java @@ -32,7 +32,6 @@ import static org.mockito.Mockito.doCallRealMethod; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.same; import static org.mockito.Mockito.spy; import com.hedera.block.server.config.BlockNodeContext; diff --git a/server/src/test/resources/app.properties b/server/src/test/resources/app.properties index 58893e32..40a696da 100644 --- a/server/src/test/resources/app.properties +++ b/server/src/test/resources/app.properties @@ -3,4 +3,4 @@ prometheus.endpointEnabled=false mediator.ringBufferSize=1024 notifier.ringBufferSize=1024 # todo do we need block as dir here in tests? -persistence.storage.type=BLOCK_AS_DIR \ No newline at end of file +persistence.storage.type=BLOCK_AS_LOCAL_DIRECTORY