feat: new BlockWriter implementation for block-as-file #355

ata-nas · 2024-11-19T09:27:29Z

Description:

This PR aims to introduce the new BlockAsFileWriter with the initial goal to only be able to write blocks as file. In subsequent PRs this functionality will be improved and cleaned up. You can see #309 for more details on the full functionality that will be implemented.

Related issue(s):

Fixes #281 #319

Notes for reviewer:

TBD

Checklist

make configurable the instantiation of a type of BlockWriter
make it possible to write blocks as file
cleanup

Documented (Code comments, README, etc.)
Tested (unit, integration, etc.)

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

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

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

ata-nas · 2024-11-19T15:00:28Z

Intermediate thoughts at 9fa5c10:

currently I am interested only into getting the new writer to write blocks as files (not yet implemented)
the new writer currently implements BlockWriter<List<BlockItem>> in order for me to get it to work as fast as possible, subject to change in the future with BlockWriter<Block>
introduced a PathResolver, resolving the path to a block, be it a block-as-file or a block-as-dir is a core function of the readers/writers, I extracted that logic in a new component so that it can be thoroughly tested on it's own and also to provide a lot of flexibility by allowing us to mock this functionality when we test the readers/writers itself
the PathResolver also separates the concern of resolving block paths from the reader/writer (single responsibility is improved)
the object creation (type of) of the readers, writers, removers and path resolvers is done now via a configurable value
added abstract classes, there will be reusable logic (in some places already exists) and will help us to formalize what dependencies the given service will need, regardless of specific implementation

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

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

ata-nas · 2024-11-20T10:10:42Z

Update at 5c0a707:

we now have simple logic to write the block as file to the filesystem
in this v1 commit, I aim to simply start writing to the fs
what am I missing
what additional handles I need to make
what does the cleanup at this stage looks like
what can be removed
what about cases where we receive a list of block items, but the block header and/or proof is missing, technically this means that I must have received the header previously and expect the proof later, the block stream is a continuous stream of block items delimited by block header and block proof

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

codecov · 2024-11-20T12:46:23Z

Codecov Report

Attention: Patch coverage is 54.10959% with 67 lines in your changes missing coverage. Please review.

Project coverage is 90.12%. Comparing base (ad18213) to head (6126411).

Files with missing lines	Patch %	Lines
...r/persistence/storage/write/BlockAsFileWriter.java	8.00%	23 Missing ⚠️
...server/persistence/PersistenceInjectionModule.java	34.37%	19 Missing and 2 partials ⚠️
...sistence/storage/path/BlockAsFilePathResolver.java	0.00%	8 Missing ⚠️
...rver/persistence/storage/read/NoOpBlockReader.java	0.00%	4 Missing ⚠️
...persistence/storage/path/AbstractPathResolver.java	0.00%	3 Missing ⚠️
...rsistence/storage/path/BlockAsDirPathResolver.java	0.00%	3 Missing ⚠️
...persistence/storage/remove/BlockAsFileRemover.java	0.00%	2 Missing ⚠️
...ver/persistence/storage/path/NoOpPathResolver.java	75.00%	1 Missing ⚠️
...er/persistence/storage/read/BlockAsFileReader.java	50.00%	1 Missing ⚠️
...server/persistence/storage/remove/NoOpRemover.java	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #355      +/-   ##
============================================
- Coverage     94.62%   90.12%   -4.50%     
- Complexity      316      332      +16     
============================================
  Files            69       82      +13     
  Lines          1228     1327      +99     
  Branches         84       89       +5     
============================================
+ Hits           1162     1196      +34     
- Misses           55      119      +64     
- Partials         11       12       +1

Files with missing lines	Coverage Δ
...a/com/hedera/block/common/utils/Preconditions.java	`100.00% <100.00%> (ø)`
...er/config/ServerMappedConfigSourceInitializer.java	`100.00% <100.00%> (ø)`
...edera/block/server/metrics/MetricsServiceImpl.java	`100.00% <100.00%> (ø)`
...rver/persistence/StreamPersistenceHandlerImpl.java	`100.00% <100.00%> (ø)`
.../persistence/storage/PersistenceStorageConfig.java	`100.00% <ø> (ø)`
.../block/server/persistence/storage/StorageType.java	`100.00% <100.00%> (ø)`
...istence/storage/read/BlockAsFileReaderBuilder.java	`100.00% <100.00%> (ø)`
...persistence/storage/write/AbstractBlockWriter.java	`100.00% <100.00%> (ø)`
...er/persistence/storage/write/BlockAsDirWriter.java	`100.00% <100.00%> (ø)`
...istence/storage/write/BlockAsDirWriterBuilder.java	`100.00% <100.00%> (ø)`
... and 11 more

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

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

jsync-swirlds

A few areas that could be improved.

jsync-swirlds · 2024-11-20T14:44:42Z

common/src/main/java/com/hedera/block/common/utils/Preconditions.java

+     * positive
+     */
+    public static long requirePositive(final long toCheck, final String errorMessage) {
+        if (0 >= toCheck) {


nit: 0 ==>> 0L.

jsync-swirlds · 2024-11-20T14:46:30Z

server/docs/configuration.md

@@ -16,3 +16,4 @@ defaults and can be left unchanged. It is recommended to browse the properties b
 | 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 |
 | NOTIFIER_RING_BUFFER_SIZE | Size of the ring buffer used by the notifier (must be a power of 2) | 2048 |
+| PERSISTENCE_STORAGE_TYPE | Type of the persistence storage | BLOCK_AS_FILE |


BLOCK_AS_FILE ==> BLOCK_AS_LOCAL_FILE

jsync-swirlds · 2024-11-20T15:21:31Z

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

+            @NonNull final BlockNodeContext blockNodeContext,
+            @NonNull final BlockRemover blockRemover,
+            @NonNull final PathResolver pathResolver) {
+        Objects.requireNonNull(blockNodeContext);


We can remove this line; the first line of the code will throw if this is null.

jsync-swirlds · 2024-11-20T15:23:17Z

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

+                case null -> throw new NullPointerException(
+                        "Persistence StorageType cannot be [null], cannot create an instance of BlockWriter");


This is not good. Either the type should be guaranteed non null from config, or use Objects.requireNonNull right before this switch.

What you were looking at is outdated, it was removed before your review.

jsync-swirlds · 2024-11-20T15:25:28Z

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

-    static BlockReader<Block> providesBlockReader(PersistenceStorageConfig config) {
-        return BlockAsDirReaderBuilder.newBuilder(config).build();
+    static BlockReader<Block> providesBlockReader(@NonNull final PersistenceStorageConfig config) {
+        final StorageType persistenceType = Objects.requireNonNull(config).type();


Objects.requireNonNull is unnecessary if the call would throw NPE anyway, and the native throw will usually produce better information.

Suggested change

final StorageType persistenceType = Objects.requireNonNull(config).type();

final StorageType persistenceType = config.type();

jsync-swirlds · 2024-11-25T22:00:40Z

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

+        } catch (final Exception e) {
+            // todo handle properly
+            throw new RuntimeException(e);
+        }


Unless you have a very good reason, do not catch Exception, ever (a vast array of bugs in real systems come from this seemingly innocuous block).
Instead, handle any specific exceptions you can, declare any checked exceptions necessary, and let callers handle things from there.

The try-catch here would probably be greatly reworked or removed from here. This was done just to help develop the core functionality.

jsync-swirlds · 2024-11-25T22:01:30Z

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

+            throw new RuntimeException(e);
+        }
+        curentBlock = null;
+        return Optional.of(blockToWrite.items());


Why do we return an Optional if we never return an empty value?

It is true, at this point, we do not really need to, the place where I call this method requires the Optional wrapping since I return it immediately and the API returns Optional, but yes, the wrapping could be done by the caller of this method, rather than proactively by the method.

jsync-swirlds · 2024-11-25T22:03:13Z

...rc/main/java/com/hedera/block/server/persistence/storage/write/BlockAsFileWriterBuilder.java

+     * @param blockPathResolver valid, {@code non-null} instance of {@link BlockPathResolver}
+     * @return a new instance of {@link BlockAsFileWriterBuilder}
+     */
+    public static BlockAsFileWriterBuilder newBuilder(


Builder constructors should not, in general, take parameters. The whole point of a "builder" is to start empty and set each value as needed (chained by returning the builder from each set method) before a build produces the result value.

What you've written here is an oddly structured factory, rather than a builder.

I agree about the builders, in fact, I prefer to implement Joshua Bloch's builder where we have Builder class that is internal to the class we want to build, since we can access directly the private filed of the internal builder from the target class and pass the builder itself as a constructor parameter.

I also agree that the builder should start "empty" and we set each value as needed.

I do not agree however that the builder constructor or static method that gets us the builder instance should have no arguments. There are some things that are mandatory for the given instance that will be constructed. Without forcing the caller to supply them, it is very error prone. The optional stuff should be settable later.

In our case it really looks like an odd factory, in fact I was thinking if we need the builders at all, we could just have static of() or from() or similar method directly inside the target class and omit the builders. The only place where we currently have an optional value is the folderPermissions which I will remove the possibility to be set externally, they shall remain an internal property of the class. All other builders are in the same fashion.

There are two views here in my opinion.

We simplify and remove the builders as they are not needed at all at this point, no need to support them.

We keep the builders as they are with the thinking that if in the future will appear optional dependencies they will be introduced within the builder as setters and our refactoring will be minimal.

UNRELATED SIDE NOTE: In some rare cases where we have a very complex building logic that requires some things to be set before others or when setting something then we must proceed with additional settings for what we just set, then I like to do interactive builders where I have multiple interfaces with specific setters and my builder implements all giving me the possibility to guide the caller through the possible options at any given step, returning a specific interface. (not applicable at all to us, just an interesting side note)

jsync-swirlds · 2024-11-25T22:05:20Z

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

     * @return an optional containing the block item written to storage if the block item was a
     *     block proof signaling the end of the block, an empty optional otherwise.
     * @throws IOException when failing to write the block item to storage.
     */
-    Optional<V> write(@NonNull final V blockItem) throws IOException;
+    Optional<V> write(@NonNull final V toWrite) throws IOException;


nit: toWrite feels a bit short. itemToWrite reads better.

It is not necessary an item, sometimes it could be a Block (not present today) or as it is today List<BlockItem> which is plural and still does not match the semantics. I rely on the parameter's type to supplement the semantics for what we are writing, so whatever the type will be, it will always be followed by toWrite.

jsync-swirlds · 2024-11-25T22:07:16Z

server/src/main/resources/app.properties

+
+#Persistence Storage Config
+#persistence.storage.rootPath=
+#persistence.storage.type=BLOCK_AS_DIR


Remember to include newlines at EOF.

Hmm should have been done automatically by my IDE, Ill take look, thanks for noticing!

On a side note, is it not possible to ensure this with spotless? Would be good if we can do that.

ata-nas · 2024-11-26T08:31:30Z

@jsync-swirlds thanks for the review, I have missed a couple of places where I could clean up additionally, however at some places it looks like you were looking at older changes, files were amended before you have made your review. I have also included #319 here and will bring additional changes to the exception handling to clean that up as well.

…-as-file

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

ata-nas added the Block Node Issues/PR related to the Block Node. label Nov 19, 2024

ata-nas added this to the 0.3.0 milestone Nov 19, 2024

ata-nas self-assigned this Nov 19, 2024

ata-nas linked an issue Nov 19, 2024 that may be closed by this pull request

feat: new BlockWriter implementation for block-as-file #281

Open

ata-nas added 2 commits November 19, 2024 14:45

WIP adding BlockWriter conditional creation and placeholder logic

a73f4f9

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

WIP adding more creation logic, cleanup required after seein what wil…

2d82213

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

ata-nas force-pushed the 281-new-blockwriter-implementation-for-block-as-file branch from 1e81bdb to 2d82213 Compare November 19, 2024 12:45

ata-nas added 2 commits November 19, 2024 15:16

WIP minor cleanup

827db48

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

WIP introducing PathResolver

c30b391

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

ata-nas added 2 commits November 19, 2024 17:08

WIP adding storage type env var configuration support

9fa5c10

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

WIP simple writing of block files to the fs, need to handle cases and…

5c0a707

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

WIP proof of concept write blocks as file to disk

6126411

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

ata-nas added 4 commits November 21, 2024 11:39

WIP abstract writer becomes local writer interface

945f514

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

WIP some refactor and additions

be28196

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

WIP javadoc improvements and cleanup

927c5ec

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

WIP smoke tests use BLOCK_AS_DIR

d1094d5

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

jsync-swirlds reviewed Nov 25, 2024

View reviewed changes

ata-nas added 3 commits November 26, 2024 11:34

Merge branch 'main' into 281-new-blockwriter-implementation-for-block…

5f5a435

…-as-file

WIP additional changes after merge

2c4fc8b

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

WIP address some pr comments

5daabfb

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new BlockWriter implementation for block-as-file #355

feat: new BlockWriter implementation for block-as-file #355

ata-nas commented Nov 19, 2024 •

edited

Loading

ata-nas commented Nov 19, 2024 •

edited

Loading

ata-nas commented Nov 20, 2024 •

edited

Loading

codecov bot commented Nov 20, 2024

jsync-swirlds left a comment

jsync-swirlds Nov 20, 2024

ata-nas Nov 26, 2024

jsync-swirlds Nov 20, 2024

ata-nas Nov 26, 2024

jsync-swirlds Nov 20, 2024

ata-nas Nov 26, 2024

jsync-swirlds Nov 20, 2024

ata-nas Nov 26, 2024

jsync-swirlds Nov 20, 2024

ata-nas Nov 26, 2024

jsync-swirlds Nov 25, 2024

ata-nas Nov 26, 2024

jsync-swirlds Nov 25, 2024

ata-nas Nov 26, 2024

ata-nas Nov 26, 2024

jsync-swirlds Nov 25, 2024

ata-nas Nov 26, 2024 •

edited

Loading

jsync-swirlds Nov 25, 2024

ata-nas Nov 26, 2024

jsync-swirlds Nov 25, 2024

ata-nas Nov 26, 2024

ata-nas Nov 26, 2024

ata-nas commented Nov 26, 2024

		case null -> throw new NullPointerException(
		"Persistence StorageType cannot be [null], cannot create an instance of BlockWriter");

	final StorageType persistenceType = Objects.requireNonNull(config).type();
	final StorageType persistenceType = config.type();

feat: new BlockWriter implementation for block-as-file #355

Are you sure you want to change the base?

feat: new BlockWriter implementation for block-as-file #355

Conversation

ata-nas commented Nov 19, 2024 • edited Loading

ata-nas commented Nov 19, 2024 • edited Loading

ata-nas commented Nov 20, 2024 • edited Loading

codecov bot commented Nov 20, 2024

Codecov Report

jsync-swirlds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ata-nas Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ata-nas commented Nov 26, 2024

ata-nas commented Nov 19, 2024 •

edited

Loading

ata-nas commented Nov 19, 2024 •

edited

Loading

ata-nas commented Nov 20, 2024 •

edited

Loading

ata-nas Nov 26, 2024 •

edited

Loading