Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 56 commits into from
Dec 6, 2024

Conversation

ata-nas
Copy link
Contributor

@ata-nas ata-nas commented Nov 19, 2024

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


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

@ata-nas ata-nas added the Block Node Issues/PR related to the Block Node. label Nov 19, 2024
@ata-nas ata-nas added this to the 0.3.0 milestone Nov 19, 2024
@ata-nas ata-nas self-assigned this Nov 19, 2024
@ata-nas ata-nas linked an issue Nov 19, 2024 that may be closed by this pull request
@ata-nas ata-nas force-pushed the 281-new-blockwriter-implementation-for-block-as-file branch from 1e81bdb to 2d82213 Compare November 19, 2024 12:45
Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
@ata-nas
Copy link
Contributor Author

ata-nas commented Nov 19, 2024

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
Copy link
Contributor Author

ata-nas commented Nov 20, 2024

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

Copy link
Member

@jsync-swirlds jsync-swirlds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few areas that could be improved.

@ata-nas
Copy link
Contributor Author

ata-nas commented Nov 26, 2024

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

@ata-nas ata-nas marked this pull request as ready for review December 3, 2024 15:19
@ata-nas ata-nas requested a review from a team as a code owner December 3, 2024 15:19
@ata-nas ata-nas requested a review from jsync-swirlds December 3, 2024 16:15
Signed-off-by: Atanas Atanasov <[email protected]>
@ata-nas ata-nas requested a review from jsync-swirlds December 4, 2024 11:57
Signed-off-by: Atanas Atanasov <[email protected]>
AlfredoG87
AlfredoG87 previously approved these changes Dec 4, 2024
Copy link
Contributor

@AlfredoG87 AlfredoG87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good job!!

@ata-nas ata-nas requested a review from AlfredoG87 December 5, 2024 11:36
@ata-nas ata-nas merged commit a278936 into main Dec 6, 2024
10 checks passed
@ata-nas ata-nas deleted the 281-new-blockwriter-implementation-for-block-as-file branch December 6, 2024 14:14
Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 98.91892% with 2 lines in your changes missing coverage. Please review.

Project coverage is 97.88%. Comparing base (4cde6a4) to head (34c1c4e).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...istence/storage/remove/BlockAsLocalDirRemover.java 80.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #355      +/-   ##
============================================
+ Coverage     97.77%   97.88%   +0.11%     
- Complexity      343      374      +31     
============================================
  Files            70       76       +6     
  Lines          1256     1323      +67     
  Branches         88       91       +3     
============================================
+ Hits           1228     1295      +67     
  Misses           19       19              
  Partials          9        9              
Files with missing lines Coverage Δ
...a/com/hedera/block/common/utils/Preconditions.java 100.00% <100.00%> (ø)
...ain/java/com/hedera/block/server/BlockNodeApp.java 100.00% <ø> (ø)
...er/config/ServerMappedConfigSourceInitializer.java 100.00% <ø> (ø)
...edera/block/server/metrics/MetricsServiceImpl.java 100.00% <100.00%> (ø)
...server/persistence/PersistenceInjectionModule.java 100.00% <100.00%> (+20.00%) ⬆️
...rver/persistence/StreamPersistenceHandlerImpl.java 100.00% <100.00%> (ø)
.../persistence/storage/PersistenceStorageConfig.java 100.00% <100.00%> (ø)
...ence/storage/path/BlockAsLocalDirPathResolver.java 100.00% <100.00%> (ø)
...nce/storage/path/BlockAsLocalFilePathResolver.java 100.00% <100.00%> (ø)
...ersistence/storage/path/NoOpBlockPathResolver.java 100.00% <100.00%> (ø)
... and 8 more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Block Node Issues/PR related to the Block Node.
Projects
None yet
4 participants