WIP address some pr comments

Signed-off-by: Atanas Atanasov <[email protected]>

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;

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

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 |

server/src/main/java/com/hedera/block/server/persistence/PersistenceInjectionModule.java

@@ -68,7 +68,6 @@ static BlockWriter<List<BlockItemUnparsed>> 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<List<BlockItemUnparsed>> 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<List<BlockItemUnparsed>> providesBlockWriter(
     @Provides
     @Singleton
     static BlockReader<BlockUnparsed> 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<BlockUnparsed> 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();
         };
     }
 

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
     }
 }

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?
     }
 }

server/src/main/java/com/hedera/block/server/persistence/storage/write/BlockAsFileWriter.java

@@ -104,7 +104,8 @@ private List<BlockItemUnparsed> 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

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
+#persistence.storage.type=BLOCK_AS_LOCAL_DIRECTORY

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);

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,15 +49,15 @@ 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());
     }
 
     @Test
     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());
     }
 

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;

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
+persistence.storage.type=BLOCK_AS_LOCAL_DIRECTORY