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: configuration to use Platform SDK across all project #101

Merged
merged 23 commits into from
Aug 14, 2024
Merged

refactor: configuration to use Platform SDK across all project #101

merged 23 commits into from
Aug 14, 2024

Conversation

AlfredoG87
Copy link
Contributor

@AlfredoG87 AlfredoG87 commented Aug 14, 2024 •
edited
Loading

Description:

  • Created the following config classes
    • ConsumerConfig
    • PersistenceStorageConfig
  • Refactored Classes to use those config instead of Helidon config
  • Simplified Classes that already had BlockNodeContext to use config from there directly
  • Refactored and Simplified all related Unit Tests

Related issue(s):

Fixes #98
Fixes #99
Fixes #33

Notes for reviewer:

Checklist

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

@AlfredoG87
created a custom config data for consumer, with a single property: ti…
6f79cc4 
…meoutThresholdMillis

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
deleted unneeded application.yaml props file.
eb7667d 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
adding test app.properties for tests, with prometheus endpoint disabl…
3bc6e9a 
…ed since is not needed and is littering the logs with errors

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
added scaffolding pieces for tests that need context and configuratio…
e32db20 
…n, so providing custom properties is possible, and also, getting a test context with test config is easier.

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
Changed Consumer package to use ConsumerConfig instead of timeoutThre…
becb521 
…sholdMillis, all needed changes from server to tests

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
style fixes
67e4156 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
Refactor Persistence Storage to use PersistenceStorageConfig
736f5cb 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
improvements
9fd4a32 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
style fixes and removal of comment and log
bd13246 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87 AlfredoG87 self-assigned this Aug 14, 2024
@AlfredoG87 AlfredoG87 added the Improvement Code changes driven by non business requirements label Aug 14, 2024
@AlfredoG87 AlfredoG87 added this to the 0.1.0 milestone Aug 14, 2024
@AlfredoG87
readme changes and improvements
915379e 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
some variables refactor and improvements generally
059b234 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
refactored BlockStreamService to send the whole context instead of ju…
9d454db 
…st the ConsumerConfig

For now, only the config is useful but in the future we will include metrics as well

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
style fix
e4a8ebf 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87 AlfredoG87 marked this pull request as ready for review August 14, 2024 17:31
@AlfredoG87 AlfredoG87 requested review from a team as code owners August 14, 2024 17:31
@AlfredoG87
more improvements to README.md
3ba979a 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
fixed Typo
faf74d5 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
removed leftover comments
79014d2 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
delete app.properties in test resources in favor of test.app.properti…
759f3c8 
…es that is more descriptive.

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
return to app.properties
4a720e1 
Signed-off-by: Alfredo Gutierrez <[email protected]>
mattp-swirldslabs
mattp-swirldslabs reviewed Aug 14, 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. Ping me to review again when you're ready

server/src/main/java/module-info.java Outdated Show resolved Hide resolved
server/src/test/java/com/hedera/block/server/BlockStreamServiceTest.java Outdated Show resolved Hide resolved
server/src/main/java/com/hedera/block/server/persistence/storage/write/BlockAsDirWriter.java Show resolved Hide resolved
server/src/main/java/com/hedera/block/server/config/BlockNodeConfigExtension.java Show resolved Hide resolved
server/src/test/java/com/hedera/block/server/config/TestConfigBuilder.java Show resolved Hide resolved
@AlfredoG87
Refactored Util to FileUtils on persistence package.
1f5713f 
Added the `createPathIfNotExists` that is used in both the PersistenceStorageConfig and BlockAsDirWriter.

Refactored needed tests.

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
JavaDocs improvements
ec5cca9 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
style fix
26aa6f5 
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
removed left comment
60678e9 
Signed-off-by: Alfredo Gutierrez <[email protected]>
mattp-swirldslabs
mattp-swirldslabs approved these changes Aug 14, 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!

@AlfredoG87 AlfredoG87 merged commit 7f48aec into main Aug 14, 2024
7 checks passed
@AlfredoG87 AlfredoG87 deleted the config-refactor branch August 14, 2024 21:50
@codecov Codecov
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (87a1ac3) to head (60678e9).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff             @@
##                main      #101   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       110       114    +4     
===========================================
  Files             19        21    +2     
  Lines            414       430   +16     
  Branches          23        25    +2     
===========================================
+ Hits             414       430   +16
Files Coverage Δ
...va/com/hedera/block/server/BlockStreamService.java 100.00% <ø> (ø)
.../block/server/config/BlockNodeConfigExtension.java 100.00% <100.00%> (ø)
...m/hedera/block/server/consumer/ConsumerConfig.java 100.00% <100.00%> (ø)
...erver/consumer/ConsumerStreamResponseObserver.java 100.00% <100.00%> (ø)
.../block/server/mediator/LiveStreamMediatorImpl.java 100.00% <ø> (ø)
...ra/block/server/persistence/storage/FileUtils.java 100.00% <100.00%> (ø)
.../persistence/storage/PersistenceStorageConfig.java 100.00% <100.00%> (ø)
...ver/persistence/storage/read/BlockAsDirReader.java 100.00% <100.00%> (ø)
...sistence/storage/read/BlockAsDirReaderBuilder.java 100.00% <100.00%> (ø)
...er/persistence/storage/write/BlockAsDirWriter.java 100.00% <100.00%> (ø)
... and 1 more

@AlfredoG87 AlfredoG87 mentioned this pull request Aug 14, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@tomzhenghedera tomzhenghedera Awaiting requested review from tomzhenghedera tomzhenghedera was automatically assigned from hashgraph/release-engineering

@nathanklick nathanklick Awaiting requested review from nathanklick nathanklick is a code owner automatically assigned from hashgraph/release-engineering-managers

Assignees

@AlfredoG87 AlfredoG87

Labels
Improvement Code changes driven by non business requirements
Projects
None yet
Milestone
0.1.0
2 participants
@AlfredoG87 @mattp-swirldslabs