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

feat: closed range streaming #633

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

feat: closed range streaming #633

wants to merge 8 commits into from
+1,353 −260

Conversation

mattp-swirldslabs
Copy link
Contributor

@mattp-swirldslabs mattp-swirldslabs commented Feb 10, 2025 •
edited
Loading

Reviewer Notes

  • Added support for closed-range historic block streaming as specified in the design doc
  • Modified Simulator code with an additional streaming method based on config values
  • Added range validation checks in PbjBlockStreamServiceProxy
  • Modified the consumer.sh to pass the smoke tests after the changes.
  • Added dashboard row to count historic blocks sent
  • Added debug logging for troubleshooting

Related Issue(s)

@mattp-swirldslabs mattp-swirldslabs added Block Node Issues/PR related to the Block Node. Alpha-0 Issues related to features designated for the Alpha-0 release labels Feb 10, 2025
@mattp-swirldslabs mattp-swirldslabs added this to the 0.5.0 milestone Feb 10, 2025
@mattp-swirldslabs mattp-swirldslabs self-assigned this Feb 10, 2025
ata-nas
ata-nas reviewed Feb 11, 2025
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.

Good job, looks good on first glance. I leave some initial comments, also I would personally never use var, I understand that may be more of a "code style" remark, but it is really hard to determine what the type is supposed to be.

server/src/main/java/com/hedera/block/server/consumer/HistoricBlockStreamSupplier.java Outdated Show resolved Hide resolved
server/src/main/java/com/hedera/block/server/consumer/HistoricBlockStreamSupplier.java Outdated Show resolved Hide resolved
server/src/main/java/com/hedera/block/server/consumer/HistoricBlockStreamSupplier.java Outdated
this.endBlockNumber = endBlockNumber;
this.blockReader = Objects.requireNonNull(blockReader);

final var consumerConfig = blockNodeContext.configuration().getConfigData(ConsumerConfig.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not use the block node context. We should remove this. This object is only used to do some DI for the metrics service and/or some specific config type. The metrics service should be simply injected as any other component, same as with any specific config type. There already exists a tech debt issue made to address this context object #629. Let's not introduce additional debt on this and simply inject what we need properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've switched to injecting the direct dependencies vs the BlockNodeContext.

server/src/test/java/com/hedera/block/server/pbj/PbjBlockStreamServiceProxyTest.java Outdated Show resolved Hide resolved
server/src/test/java/com/hedera/block/server/util/PersistTestUtils.java Show resolved Hide resolved
simulator/src/main/java/com/hedera/block/simulator/grpc/impl/ConsumerStreamGrpcClientImpl.java Outdated Show resolved Hide resolved
server/src/test/java/com/hedera/block/server/consumer/HistoricBlockStreamSupplierTest.java Outdated Show resolved Hide resolved
@mattp-swirldslabs mattp-swirldslabs force-pushed the 00581-closed-range-streaming branch 3 times, most recently from 5923d9d to 8328c95 Compare February 11, 2025 23:45
@mattp-swirldslabs mattp-swirldslabs marked this pull request as ready for review February 12, 2025 00:13
@mattp-swirldslabs mattp-swirldslabs requested a review from a team as a code owner February 12, 2025 00:13
@mattp-swirldslabs mattp-swirldslabs force-pushed the 00581-closed-range-streaming branch 2 times, most recently from b6b183c to 0e6bff6 Compare February 13, 2025 21:47
AlfredoG87
AlfredoG87 previously approved these changes Feb 14, 2025
View reviewed changes
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.

Great work! Looks good! 💯
just 1 nit.

server/src/main/java/com/hedera/block/server/config/ServerMappedConfigSourceInitializer.java
@@ -16,6 +16,7 @@ public final class ServerMappedConfigSourceInitializer {

// Consumer Config
new ConfigMapping("consumer.timeoutThresholdMillis", "CONSUMER_TIMEOUT_THRESHOLD_MILLIS"),
new ConfigMapping("consumer.maxBlockItemBatchSize", "CONSUMER_MAX_BLOCK_ITEM_BATCH_SIZE"),
Copy link
Contributor

Choose a reason for hiding this comment

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

are we still maintaining docs for configs?
I think new property is missing there.

https://github.com/hashgraph/hedera-block-node/blob/main/server/docs/configuration.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch...I didn't even realize we had this document. I added a field for it.

@mattp-swirldslabs
feat: closed-range historic block streaming
65b12b5 
Signed-off-by: Matt Peterson <[email protected]>
@mattp-swirldslabs
fix: adjusted the consumer script to fix the smoke tests
576fc0c 
Signed-off-by: Matt Peterson <[email protected]>
@mattp-swirldslabs
fix: changed to use chunkify and a fix to the consumer for the smoke …
5e38d68 
…test

Signed-off-by: Matt Peterson <[email protected]>
@mattp-swirldslabs
fix: fixed the streaming case when the server does not have any block…
222c2bc 
… items

Signed-off-by: Matt Peterson <[email protected]>
@mattp-swirldslabs
fix: added test coverage
8b9953a 
Signed-off-by: Matt Peterson <[email protected]>
@mattp-swirldslabs
fix: folded up all of the dashboard rows
6bab0ef 
Signed-off-by: Matt Peterson <[email protected]>
@mattp-swirldslabs
fix: pr feedback
17f9230 
Signed-off-by: Matt Peterson <[email protected]>
@mattp-swirldslabs
fix: added property documentation
0e96550 
Signed-off-by: Matt Peterson <[email protected]>
@mattp-swirldslabs mattp-swirldslabs force-pushed the 00581-closed-range-streaming branch from c1a0428 to 0e96550 Compare February 14, 2025 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rbair23 rbair23 rbair23 left review comments

@ata-nas ata-nas ata-nas left review comments

@AlfredoG87 AlfredoG87 AlfredoG87 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@mattp-swirldslabs mattp-swirldslabs

Labels
Alpha-0 Issues related to features designated for the Alpha-0 release Block Node Issues/PR related to the Block Node.
Projects
None yet
Milestone
0.5.0
Development

Successfully merging this pull request may close these issues.

Unit test closed-range historic streaming Implement closed-range historic streaming
4 participants
@mattp-swirldslabs @rbair23 @ata-nas @AlfredoG87