feat: new BlockWriter implementation for block-as-file

[ata-nas]

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 #348

Notes for reviewer:

TBD

Checklist

• make configurable the instantiation of a type of BlockWriter
• make it possible to write blocks as file
• fix refactor: BlockAsDirReader and BlockAsDirWriter review and overhaul #319
• fix chore: replace large catch block with a method call repairPermissionsForRetry #348
• cleanup

---

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

[ata-nas]

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

[ata-nas]

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

[codecov]

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

[jsync-swirlds]

A few areas that could be improved.

[jsync-swirlds]

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.

[ata-nas]

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]

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.

[ata-nas]

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.

1. We simplify and remove the builders as they are not needed at all at this point, no need to support them.
2. 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]

Thank you for the well reasoned reply. Here are my thoughts.

1. Removing the builders and using a static of method makes sense in these small-value cases where you want all the required values set. My comment was that a constructor, specifically, should not have parameters; a static method is a secondary pattern (builder-factory-method), and it is fine to have parameters (in fact practically required) there.
   • It also makes sense to have a builder that is properly designed, internal to the class to build, and accessible only as a return value from an of method that takes the required parameters; this also avoids the perceived need to have a constructor with parameters while retaining regular "builder" semantics.
2. Re: the side note, I am not a huge fan of interactive builders with multiple interfaces (I've seen that become a nest of bugs that breeds more too many times). If there is an ordering or containment requirement, the intent of a builder would, in my opinion, be to remove the ordering/containment requirement and allow values to be set in any order, but then apply them to the created object in the correct order and containment during the build method. A build method might, in this setup, construct an entire tree of objects in order to meet all the requirements for a particular highly complex object build.

[ata-nas]

@jsync-swirlds awesome! Thanks also for your insights! I like the combination of the of method that will get us an internal builder. I will implement that, it will clean up a lot of things! Just so that there is no confusion on my side:

1. Do you propose that I will now remove the external builders and have a static of method that will be part of the target class, from where we will supply all mandatory dependencies AND since there are currently no cases (or will not be once I remove the folderPermissions) of optional dependencies, the of method alone is sufficient. Once we have an optional dependency for the given class, the of method will then return a newly made internal to the target class "Joshua Bloch" style builder and it will be an easy refactor.

OR

2. Same as above, I remove the external builders, have an of method in the target class, but proactively create the internal builders and simply call the build method where I want an instance of the target class, despite the fact that currently we have no optional deps.

?

In any case, I am generally pro for static instance creation methods.

Let me know, thanks again, insightful thread! :)

[jsync-swirlds]

I would recommend starting with option (1), and we can always adjust later if needed.

[ata-nas]

✅ Done. Quite the refactoring it was, took a while, but everything is a lot cleaner :). One note, for the places where are no input params, I have used the method name 'newInstnace', the name 'of' and then passing nothing really did not fit at all. Since now everything is wired, a simple rename of all usages of 'newInstance' is trivial and takes milliseconds.

[ata-nas]

@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.