feat: task based archiving #659

ata-nas · 2025-02-18T13:38:23Z

Reviewer Notes

introduce a task-based archiver
the new archiver is currently part of the persistence service (this is due to change in the future and the archiver will be a standalone component, need additional infrastructure)
updates to reading logic to accommodate proper zip entry naming
currently we rely on gap between latest live written and what will be zipped for proper operation due to lack of certain infrastructure
in another issue need to be handled (out of scope for this one)
- ability to stop a long running archive
- have a process/thread that would be able to go through and find out all the roots that have failed archiving and attempt to archive them again
- ensure group size cannot be changed once set the first time (restart should not be possible with a different setting once the roots are created)

Related Issue(s)

Closes #517

server/src/main/java/com/hedera/block/server/ack/AckHandlerImpl.java

server/src/main/java/com/hedera/block/server/config/ServerMappedConfigSourceInitializer.java

mattp-swirldslabs

Nice work. Extensive and detailed javadoc and comments!

server/docker/logging.properties

server/src/main/java/com/hedera/block/server/config/ServerMappedConfigSourceInitializer.java

mattp-swirldslabs · 2025-02-20T22:44:23Z

server/src/main/java/com/hedera/block/server/persistence/storage/PersistenceStorageConfig.java

@@ -52,6 +52,9 @@ public record PersistenceStorageConfig(
     */
    public PersistenceStorageConfig {
        Objects.requireNonNull(type);
+        if (archiveBatchSize <= 1) {
+            throw new IllegalArgumentException("The archive group size must be greater than [1]");
+        }


Could we use Preconditions.requireGreaterOrEqual(2, archiveBatchSize) here?

Really this should just require >0 because we also test that the batch is a power of 10 on the next line.

✅ Done, the precondition is improved.

server/src/main/resources/app.properties

...er/src/test/java/com/hedera/block/server/config/ServerMappedConfigSourceInitializerTest.java

mattp-swirldslabs · 2025-02-20T23:22:54Z

I tried attempted to start the container but it fails with /bin/bash: line 1: /app/server-0.5.0-SNAPSHOT/bin/server: No such file or directory. Is that something you're still sorting out?

jsync-swirlds

About 1/3 finished.

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

.../java/com/hedera/block/server/persistence/storage/archive/AsyncBlockAsLocalFileArchiver.java

jsync-swirlds · 2025-02-20T22:24:42Z

...rc/main/java/com/hedera/block/server/persistence/storage/archive/LocalBlockFileArchiver.java

+                // either a bug in the archiving task, or an unhandled case
+                throw new RuntimeException(e.getCause());
+            } catch (final InterruptedException e) {
+                // @todo(517) What shall we do here? How to handle?


Most likely, what's here is all we need to do.
We might call something like shutdownNow on the executor, if we have a reference. Otherwise just resetting the interrupt flag for this thread should be sufficient.

.../java/com/hedera/block/server/persistence/storage/archive/AsyncBlockAsLocalFileArchiver.java

jsync-swirlds · 2025-02-20T23:03:41Z

.../java/com/hedera/block/server/persistence/storage/archive/AsyncBlockAsLocalFileArchiver.java

+        try (final Stream<Path> pathsToDelete = Files.walk(movedToDelete)) {
+            pathsToDelete.sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);
+        }


A 150ms sleep (or some other mechanism to delay the removal a bit) before deleting is a wise precaution to ensure that the files and folders are all closed before we try to remove them. That's needed to reduce the risk an IOException will be thrown just because some other thread hasn't ended in time.
Perhaps a better approach would be to scan for open files/folders in the path, but that's likely to take quite a bit longer than just sleeping a little...

jsync-swirlds · 2025-02-20T23:07:40Z

server/src/main/java/com/hedera/block/server/persistence/storage/PersistenceStorageConfig.java

@@ -52,6 +52,9 @@ public record PersistenceStorageConfig(
     */
    public PersistenceStorageConfig {
        Objects.requireNonNull(type);
+        if (archiveBatchSize <= 1) {
+            throw new IllegalArgumentException("The archive group size must be greater than [1]");
+        }


Really this should just require >0 because we also test that the batch is a power of 10 on the next line.

jsync-swirlds · 2025-02-20T23:10:11Z

server/src/main/java/com/hedera/block/server/persistence/storage/PersistenceStorageConfig.java

+        /**
+         * This type of archive does nothing, essentially acting as if it were
+         * disabled.
+         */
+        NO_OP


Given this is just for testing, I wonder if we just remove the type configuration and only use the no-op implementation via injection in tests.

We shouldn't plan on creating more archive options and choosing between them based on configuration...

.../java/com/hedera/block/server/persistence/storage/archive/AsyncBlockAsLocalFileArchiver.java

- introduce a task-based archiver - the new archiver is currently part of the persistence service (this is due to change in the future and the archiver will be a standalone component, need additional infrastructure) - updates to reading logic to accommodate proper zip entry naming - currently we rely on gap between latest live written and what will be zipped for proper operation due to lack of certain infrastructure - tests for new logic - live and archive roots are created by the persistence handler Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>

- chagned zip out stream method to STORED - changed zip out stream level to NO_COMPRESSION - changed each individual zip entry method to STORED Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>

- deletion of unnecessary classes/wrappers and simplification of the archivers Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>

Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>

…nd proper naming for archiver task Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>

Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>

ata-nas · 2025-02-21T14:28:03Z

I tried attempted to start the container but it fails with /bin/bash: line 1: /app/server-0.5.0-SNAPSHOT/bin/server: No such file or directory. Is that something you're still sorting out?

@mattp-swirldslabs to answer your question, from the previous chore that was just merged in main #371, there the roots were created in two places, which was wrong, but that was to be immediately fixed with this pr. Now both roots are created in the proper place and you should be able to start it without any issues. It was a matter of rebasing.

…ckhandler get or create from map Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>

jsync-swirlds

Some additional suggestions.

Also, as a side note, when we get to where we're trying to read these files, we should look at the java zip filesystem so we can treat the zip file just like it's the folder it replaced...

jsync-swirlds · 2025-02-21T18:28:46Z

...c/main/java/com/hedera/block/server/persistence/storage/archive/BlockArchivingException.java

+/**
+ * A simple exception class to represent an error during block archiving.
+ */
+public final class BlockArchivingException extends RuntimeException {


When creating exceptions, it is important to always include all four standard constructors:

BlockArchivingException() BlockArchivingException(String message) BlockArchivingException(Throwable cause) BlockArchivingException(String message, Throwable cause)

This keeps exception throwing semantics consistent and reliable.

jsync-swirlds · 2025-02-21T18:33:43Z