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.

Sign up for GitHub

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

Jump to bottom

refactor: Stream of repeated block items instead of block items #269

Merged
merged 15 commits into from
Oct 22, 2024
Merged

refactor: Stream of repeated block items instead of block items #269

merged 15 commits into from
Oct 22, 2024

Conversation

AlfredoG87
Copy link
Contributor

@AlfredoG87 AlfredoG87 commented Oct 16, 2024 •
edited
Loading

Description:

This PR changes the version of hedera-protobuf and refactors all the project due to 2 fundamental changes to the API spec.

  1. Service was changed from 1 single service called BlockStreamService to 4 different services: BlockStreamService, BlockAccessService, BlockNodeService, StateService.
  2. BlockStreamService --> publishBlockStream and subscribeBlockStream to use a repeated set of block_items per request instead of a single block_item.

Changes done to support the change:

  1. Updated stream branch/commit to use a temporal temporary branch until is merged to main.
  2. Changes to the simulator to steam by batch
  3. Changes to Helidon Interfaces and Internal Services
  4. Fixing Unit Tests to reflect changes
  5. Refactored BlockStreamService to extract BlockStreamAccess methods.
  6. Add BlockStreamAccesss Grpc Service to start-up routes.
  7. Added new and missing UTs
  8. Updated Configuration and Documentation

Related issue(s):

Fixes #261
Fixes #262
Fixes #268
Fixes #276
Fixes #277
Fixes #278
Fixes #270
Fixes #279

Notes for reviewer:
Main refactor is completed, at least is compiling :)

Checklist

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

@AlfredoG87 AlfredoG87 self-assigned this Oct 16, 2024
@AlfredoG87 AlfredoG87 added Block Node Issues/PR related to the Block Node. Improvement Code changes driven by non business requirements P2 Required to be completed in the assigned milestone, but may or may not impact release schedule. P1 High priority issue. Required to be completed in the assigned milestone. and removed P2 Required to be completed in the assigned milestone, but may or may not impact release schedule. labels Oct 16, 2024
@AlfredoG87 AlfredoG87 added this to the 0.2.0 milestone Oct 16, 2024
@AlfredoG87 AlfredoG87 force-pushed the repeated-block-items-refactor branch from e4b3ddd to d18788f Compare October 16, 2024 23:21
@AlfredoG87 AlfredoG87 changed the title Repeated block items refactor refactor : stream of repeated block items instead of block items Oct 18, 2024
@AlfredoG87 AlfredoG87 changed the title refactor : stream of repeated block items instead of block items refactor: Stream of repeated block items instead of block items Oct 18, 2024
@AlfredoG87 AlfredoG87 force-pushed the repeated-block-items-refactor branch from ea93713 to 5b096ec Compare October 18, 2024 23:19
@AlfredoG87 AlfredoG87 marked this pull request as ready for review October 21, 2024 05:01
@AlfredoG87 AlfredoG87 requested a review from a team as a code owner October 21, 2024 05:01
georgi-l95
georgi-l95 reviewed Oct 21, 2024
View reviewed changes
Copy link
Contributor

@georgi-l95 georgi-l95 left a comment

Choose a reason for hiding this comment

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

LG, some questions

server/src/main/java/com/hedera/block/server/grpc/BlockAccessService.java Show resolved Hide resolved
server/src/main/java/com/hedera/block/server/mediator/LiveStreamMediatorImpl.java Outdated Show resolved Hide resolved
ata-nas
ata-nas reviewed Oct 21, 2024
View reviewed changes
simulator/README.md Outdated Show resolved Hide resolved
mattp-swirldslabs
mattp-swirldslabs reviewed Oct 21, 2024
View reviewed changes
Copy link
Contributor

@mattp-swirldslabs mattp-swirldslabs left a comment

Choose a reason for hiding this comment

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

Nice work. I added some comments

server/src/main/java/com/hedera/block/server/grpc/BlockAccessService.java Outdated Show resolved Hide resolved
server/src/main/java/com/hedera/block/server/grpc/BlockAccessService.java Show resolved Hide resolved
server/src/main/java/com/hedera/block/server/producer/ProducerBlockItemObserver.java Show resolved Hide resolved
server/src/main/java/com/hedera/block/server/grpc/BlockStreamService.java Show resolved Hide resolved
server/src/main/java/com/hedera/block/server/consumer/ConsumerStreamResponseObserver.java Outdated Show resolved Hide resolved
server/src/test/java/com/hedera/block/server/grpc/BlockAccessServiceTest.java Outdated Show resolved Hide resolved
server/src/test/java/com/hedera/block/server/persistence/storage/read/BlockAsDirReaderTest.java Outdated Show resolved Hide resolved
server/src/test/java/com/hedera/block/server/persistence/storage/read/BlockAsDirReaderTest.java Outdated Show resolved Hide resolved
server/src/test/java/com/hedera/block/server/persistence/storage/read/BlockAsDirReaderTest.java Outdated Show resolved Hide resolved
server/src/test/java/com/hedera/block/server/persistence/storage/read/BlockAsDirReaderTest.java Outdated Show resolved Hide resolved
ata-nas
ata-nas reviewed Oct 22, 2024
View reviewed changes
Copy link
Contributor

@ata-nas ata-nas left a comment

Choose a reason for hiding this comment

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

Looking better! A couple comments on latest changes only reviewed.

common/src/main/java/com/hedera/block/common/utils/ChunkUtils.java Show resolved Hide resolved
common/src/main/java/com/hedera/block/common/utils/ChunkUtils.java Outdated Show resolved Hide resolved
simulator/build.gradle.kts Outdated Show resolved Hide resolved
server/src/test/java/com/hedera/block/server/grpc/BlockAccessServiceTest.java Show resolved Hide resolved
server/src/test/java/com/hedera/block/server/grpc/BlockStreamServiceTest.java Show resolved Hide resolved
server/src/test/java/com/hedera/block/server/persistence/storage/read/BlockAsDirReaderTest.java Show resolved Hide resolved
@AlfredoG87
temporary commit to use protobuf from temporal branch block-repeated-…
81c2147 
…fix-tss-message, hopefully by the time this ready to merge to main, we can use a specific commit hash

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
simple changes to allow the simulator to stream List<BlockItems> inst…
3b2be50 
…ead of individual block items.

Pending to add a config with the desired batch size.
Pending to add more tests with different batch sizes.

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
Changes to Helidon and Services
da2cc06 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
Fixing Unit Tests
27a259a 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
Refactor BlockStreamService to extract BlockAccessService
544dd6b 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
Enable BlockAccessService as part of the GRPC Routes in the BlockNode…
d6a80bf 
…App, fixed scripts for smoke-test and created an DI Injection Module for GRPC Services.

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
Added missing UT to complete coverage
2d77799 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
- Refactored BlockStreamConfig and extracted BlockGeneratorConfig Pro…
d59bcaf 
…perties to its own record class.

- Added generator.blockItemsBatchSize that defines the size of the batch to send.

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
- More fixes to the simulator and it's tests.
bf75e9c 
- Updated the README.md and app.properties example file.
- Simplify some logic

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
added missing UT
7086c45 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
Addressing PR Review comments, suggestions and improvements.
a0dd2c4 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
restoring app.properties to defaults
ea98ce3 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
Addressing more feedback from PR Reviews
0092362 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
addressing warnings and adding unit test for Chunkify Util
5369431 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87 AlfredoG87 force-pushed the repeated-block-items-refactor branch from 0794160 to 5369431 Compare October 22, 2024 17:07
@AlfredoG87
replaced with single import
925b9f7 
Signed-off-by: Alfredo Gutierrez <[email protected]>
ata-nas
ata-nas approved these changes Oct 22, 2024
View reviewed changes
Copy link
Contributor

@ata-nas ata-nas left a comment

Choose a reason for hiding this comment

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

Looks good to me, all comments I had on everything I managed to find is now resolved. Please see others as well! 👍

mattp-swirldslabs
mattp-swirldslabs approved these changes Oct 22, 2024
View reviewed changes
Copy link
Contributor

@mattp-swirldslabs mattp-swirldslabs left a comment

Choose a reason for hiding this comment

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

Looks good

jsync-swirlds
jsync-swirlds approved these changes Oct 22, 2024
View reviewed changes
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 items, most can be addressed in future PRs.

common/src/main/java/com/hedera/block/common/utils/ChunkUtils.java Show resolved Hide resolved
common/src/main/java/com/hedera/block/common/utils/ChunkUtils.java Show resolved Hide resolved
common/src/main/java/com/hedera/block/common/utils/ChunkUtils.java Show resolved Hide resolved
server/src/main/java/com/hedera/block/server/consumer/ConsumerStreamResponseObserver.java Show resolved Hide resolved
server/src/main/java/com/hedera/block/server/consumer/ConsumerStreamResponseObserver.java Show resolved Hide resolved
server/src/main/java/com/hedera/block/server/persistence/storage/write/BlockAsDirWriter.java Show resolved Hide resolved
server/src/test/java/com/hedera/block/server/consumer/ConsumerStreamResponseObserverTest.java Show resolved Hide resolved
server/src/test/resources/block_service.proto Show resolved Hide resolved
stream/build.gradle.kts Show resolved Hide resolved
@AlfredoG87 AlfredoG87 merged commit 4fb471c into main Oct 22, 2024
13 checks passed
@AlfredoG87 AlfredoG87 deleted the repeated-block-items-refactor branch October 22, 2024 18:53
This was referenced Oct 22, 2024
Closed
Closed
Closed
Closed
@codecov Codecov
Copy link

codecov bot commented Oct 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.62%. Comparing base (f5f1ed2) to head (925b9f7).
Report is 3 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main     #269      +/-   ##
============================================
+ Coverage     99.59%   99.62%   +0.03%     
- Complexity      272      290      +18     
============================================
  Files            54       57       +3     
  Lines           981     1060      +79     
  Branches         70       77       +7     
============================================
+ Hits            977     1056      +79     
  Misses            3        3              
  Partials          1        1
Files with missing lines Coverage Δ
...java/com/hedera/block/common/utils/ChunkUtils.java 100.00% <100.00%> (ø)
...ain/java/com/hedera/block/server/BlockNodeApp.java 100.00% <100.00%> (ø)
...dera/block/server/BlockNodeAppInjectionModule.java 100.00% <ø> (ø)
...erver/consumer/ConsumerStreamResponseObserver.java 100.00% <100.00%> (ø)
...m/hedera/block/server/grpc/BlockAccessService.java 100.00% <100.00%> (ø)
...m/hedera/block/server/grpc/BlockStreamService.java 100.00% <100.00%> (ø)
.../block/server/mediator/LiveStreamMediatorImpl.java 100.00% <100.00%> (ø)
...com/hedera/block/server/notifier/NotifierImpl.java 100.00% <100.00%> (ø)
...server/persistence/PersistenceInjectionModule.java 100.00% <ø> (ø)
...rver/persistence/StreamPersistenceHandlerImpl.java 100.00% <100.00%> (ø)
... and 15 more

@ata-nas ata-nas mentioned this pull request Oct 29, 2024
Merged
2 tasks
@ata-nas ata-nas mentioned this pull request Nov 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@georgi-l95 georgi-l95 georgi-l95 left review comments

@jsync-swirlds jsync-swirlds jsync-swirlds approved these changes

@mattp-swirldslabs mattp-swirldslabs mattp-swirldslabs approved these changes

@ata-nas ata-nas ata-nas approved these changes

Assignees

@AlfredoG87 AlfredoG87

Labels
Block Node Issues/PR related to the Block Node. Improvement Code changes driven by non business requirements P1 High priority issue. Required to be completed in the assigned milestone.
Projects
None yet
Milestone
0.2.0
5 participants
@AlfredoG87 @georgi-l95 @ata-nas @mattp-swirldslabs @jsync-swirlds