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

New Record Stream Facility #6469

Merged
merged 7 commits into from Jul 19, 2023
Merged

New Record Stream Facility #6469

merged 7 commits into from Jul 19, 2023

Conversation

jasperpotts
Copy link
Member

@jasperpotts jasperpotts commented May 5, 2023 •
edited

Creating new record stream writing Facility, Issue #4742

@jasperpotts jasperpotts self-assigned this May 5, 2023
@david-bakin-sl
Copy link
Member

BTW, over at a different PR dealing with stream files I ask a question about the "simple checksum" that involves the magic number 101 apparently pulled out of thin air. My ask: highlight this in the code so that in the future we can find an explanation.

@jasperpotts jasperpotts force-pushed the record-manager-v1 branch 3 times, most recently from 9c479c7 to cbc000c Compare May 26, 2023 17:40
@jasperpotts jasperpotts force-pushed the record-manager-v1 branch 4 times, most recently from c5fd9ce to 5c473a7 Compare June 2, 2023 00:29
@jasperpotts jasperpotts changed the title Starting on new Record Stream Facility New Record Stream Facility Jun 16, 2023
@jasperpotts
Copy link
Member Author

BTW, over at a different PR dealing with stream files I ask a question about the "simple checksum" that involves the magic number 101 apparently pulled out of thin air. My ask: highlight this in the code so that in the future we can find an explanation.

I have no idea of that odd checksum. Seems not very useful, would love to remove it.

rbair23
rbair23 requested changes Jun 19, 2023
View reviewed changes
hedera-node/hedera-app-spi/src/main/java/com/hedera/node/app/spi/info/NodeInfo.java Outdated Show resolved Hide resolved
...ra-app-spi/src/main/java/com/hedera/node/app/spi/records/SingleTransactionRecordBuilder.java Outdated Show resolved Hide resolved
hedera-node/hedera-app/src/main/java/com/hedera/node/app/info/MonoNodeInfo.java Outdated Show resolved Hide resolved
hedera-node/hedera-app/src/main/java/com/hedera/node/app/records/BlockRecordManager.java Outdated Show resolved Hide resolved
hedera-node/hedera-app/src/main/java/com/hedera/node/app/records/files/RecordFileFormat.java Outdated Show resolved Hide resolved
hedera-node/hedera-app/src/main/java/com/hedera/node/app/records/files/RecordFileWriter.java Outdated Show resolved Hide resolved
hedera-node/hedera-app/src/main/java/com/hedera/node/app/records/files/RecordFileWriter.java Outdated Show resolved Hide resolved
hedera-node/hedera-app/src/test/java/com/hedera/node/app/records/files/RecordStreamV6Test.java Outdated Show resolved Hide resolved
hedera-node/hedera-app/src/main/java/com/hedera/node/app/records/BlockRecordManagerImpl.java Outdated Show resolved Hide resolved
@jasperpotts jasperpotts force-pushed the record-manager-v1 branch 2 times, most recently from e37b8e2 to 08b8a68 Compare June 23, 2023 21:42
@jasperpotts jasperpotts marked this pull request as ready for review June 23, 2023 21:42
@jasperpotts jasperpotts requested review from a team as code owners June 23, 2023 21:42
@github-actions
Copy link

github-actions bot commented Jun 23, 2023 •
edited

Node: Unit Test Results

    1 558 files      1 558 suites   15m 6s ⏱️
102 903 tests 102 894 ✔️ 9 💤 0 ❌
109 299 runs  109 290 ✔️ 9 💤 0 ❌

Results for commit 0d80bfc.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Jun 23, 2023 •
edited

Codecov Report

Patch coverage: 72.72% and project coverage change: +20.93 🎉

Comparison is base (c9252b3) 68.81% compared to head (0d80bfc) 89.74%.

Additional details and impacted files 
@@              Coverage Diff               @@
##             develop    #6469       +/-   ##
==============================================
+ Coverage      68.81%   89.74%   +20.93%     
+ Complexity     25010    19911     -5099     
==============================================
  Files           2302     1574      -728     
  Lines         146837    59045    -87792     
  Branches        8921     6241     -2680     
==============================================
- Hits          101039    52990    -48049     
+ Misses         43926     4712    -39214     
+ Partials        1872     1343      -529
Impacted Files Coverage Δ
...m/hedera/node/app/spi/workflows/HandleContext.java 0.00% <ø> (ø)
...-app/src/main/java/com/hedera/node/app/Hedera.java 0.00% <0.00%> (ø)
...in/java/com/hedera/node/app/info/NodeInfoImpl.java 0.00% <0.00%> (ø)
...a/com/hedera/node/app/platform/PlatformModule.java 0.00% <0.00%> (ø)
...impl/producers/formats/v7/BlockRecordFormatV7.java 0.00% <0.00%> (ø)
...orkflows/handle/HandleWorkflowInjectionModule.java 16.66% <ø> (ø)
...app/workflows/handle/record/RecordListBuilder.java 98.24% <ø> (ø)
...ws/prehandle/PreHandleWorkflowInjectionModule.java 50.00% <ø> (ø)
...java/com/hedera/node/config/data/HederaConfig.java 50.00% <ø> (+50.00%) ⬆️
...ra/node/config/data/NetworkAdminServiceConfig.java 50.00% <ø> (ø)
... and 25 more

... and 1013 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link

github-actions bot commented Jun 23, 2023 •
edited

Node: E2E Test Results

    1 files      1 suites   17m 56s ⏱️
310 tests 310 ✔️ 0 💤 0 ❌
328 runs  328 ✔️ 0 💤 0 ❌

Results for commit 0d80bfc.

♻️ This comment has been updated with latest results.

@jasperpotts jasperpotts linked an issue Jun 26, 2023 that may be closed by this pull request
Implement improved record handling implementation #4742
Closed
rbair23
rbair23 previously approved these changes Jul 16, 2023
View reviewed changes
netopyr
netopyr reviewed Jul 17, 2023
View reviewed changes
...-app-spi/src/testFixtures/java/com/hedera/node/app/spi/fixtures/state/MapWritableStates.java Show resolved Hide resolved
...c/testFixtures/java/com/hedera/node/app/spi/fixtures/state/ObjectReadableSingletonState.java Outdated Show resolved Hide resolved
...c/testFixtures/java/com/hedera/node/app/spi/fixtures/state/ObjectWritableSingletonState.java Outdated Show resolved Hide resolved
...ra-node/hedera-app/src/main/java/com/hedera/node/app/records/BlockRecordInjectionModule.java Outdated Show resolved Hide resolved
hedera-node/hedera-app/src/main/java/com/hedera/node/app/records/BlockRecordManager.java Outdated Show resolved Hide resolved
hedera-node/hedera-app/src/main/java/com/hedera/node/app/records/BlockRecordManager.java Show resolved Hide resolved
jasperpotts and others added 5 commits July 17, 2023 16:09
@jasperpotts @rbair23
Added extra tests and fixed a couple bugs found along the way
e7c5c89 
Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>

Fixed JavaDoc

Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>

Fixed one test

Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>

Applied Spotless

Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>

Updated UtilPrngHandler to use BlockRecordInfo

Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>

Updated UtilPrngHandler to use BlockRecordInfo

Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>

Fixes to most review comments

Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>

Lots more testing and fixing, happy now

Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>

Much better testing

Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>

Spotless cleanup

Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>

Progress and tests pass :-)

Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>

Fix after rebase and moved to using Google Jimfs file system for faster tests

Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>

Added test record data

Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>

Fixing things adding tests

Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>

Lots of progress

Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>

Think I have the structure right and state

Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>

Refactored connection to workflow

Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>

Refactored for V7 and fixed HandleWorkflow relationship with BlockRecordManager
Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>
Removed need for Pbj Helper as PBJ 0.6.0 has needed API
Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>
Clean up
First working version of Record file writing.
Make everything compile after rebase.
Progress on BlockRecordManager.
Added StreamFileProducer.

Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>
@jasperpotts @rbair23
Applied Spotless
9fad578 
Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>
@jasperpotts @rbair23
Remove Commented Out Code, Fixed broken test
86bdd60 
Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>
@rbair23
Rearrange classes and deal with potential error conditions.
533c2cb 
Signed-off-by: Richard Bair <rbair23@users.noreply.github.com>
@rbair23
Update from lastBlockNo to lastBlockNumber to match latest protobuf.
102e405 
Signed-off-by: Richard Bair <rbair23@users.noreply.github.com>
@vtronkov
Copy link
Contributor

Do we want to update the records.md documentation?

netopyr
netopyr requested changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@netopyr netopyr left a comment

Choose a reason for hiding this comment

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

We should discuss in more detail how the state changes have to be committed. I think the BlockRecordManagerImpl has to be responsible to commit its own state, but I am not sure.

...a-node/hedera-app/src/main/java/com/hedera/node/app/records/impl/BlockRecordManagerImpl.java Outdated Show resolved Hide resolved
...a-node/hedera-app/src/main/java/com/hedera/node/app/records/impl/BlockRecordManagerImpl.java Show resolved Hide resolved
...a-node/hedera-app/src/main/java/com/hedera/node/app/records/impl/BlockRecordManagerImpl.java Show resolved Hide resolved
...a-node/hedera-app/src/main/java/com/hedera/node/app/records/impl/BlockRecordManagerImpl.java Outdated Show resolved Hide resolved
...a-node/hedera-app/src/main/java/com/hedera/node/app/records/impl/BlockRecordManagerImpl.java Show resolved Hide resolved
...p/src/main/java/com/hedera/node/app/records/impl/producers/StreamFileProducerConcurrent.java Show resolved Hide resolved
...p/src/main/java/com/hedera/node/app/records/impl/producers/StreamFileProducerConcurrent.java Show resolved Hide resolved
...c/main/java/com/hedera/node/app/records/impl/producers/StreamFileProducerSingleThreaded.java Show resolved Hide resolved
...src/main/java/com/hedera/node/app/records/impl/producers/formats/v6/BlockRecordFormatV6.java Show resolved Hide resolved
...src/main/java/com/hedera/node/app/records/impl/producers/formats/v6/BlockRecordFormatV6.java Outdated Show resolved Hide resolved
@rbair23
Fix unit tests. Include missing configs in ConfigProviderImpl and upd…
494ac39 
…ate HederaTestConfigBuilder to include all configs without classpath scanning.

Signed-off-by: Richard Bair <rbair23@users.noreply.github.com>
@rbair23
Apply changes from code review
0d80bfc 
Signed-off-by: Richard Bair <rbair23@users.noreply.github.com>
@rbair23 rbair23 requested a review from netopyr July 19, 2023 01:33
@sonarcloud
Copy link

sonarcloud bot commented Jul 19, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 44 Code Smells

78.9% 78.9% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

iwsimon
iwsimon approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@iwsimon iwsimon left a comment

Choose a reason for hiding this comment

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

LGTM on files owned by me.

netopyr
netopyr approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@netopyr netopyr left a comment

Choose a reason for hiding this comment

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

LGTM (except for committing state, can be handled in separate PR)

...p/src/main/java/com/hedera/node/app/records/impl/producers/StreamFileProducerConcurrent.java Show resolved Hide resolved
...p/src/main/java/com/hedera/node/app/records/impl/producers/StreamFileProducerConcurrent.java Show resolved Hide resolved
Neeharika-Sompalli
Neeharika-Sompalli approved these changes Jul 19, 2023
View reviewed changes
Copy link
Member

@Neeharika-Sompalli Neeharika-Sompalli left a comment

Choose a reason for hiding this comment

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

LGTM other than one comment

@rbair23 rbair23 merged commit e20fc85 into develop Jul 19, 2023
10 of 12 checks passed
@rbair23 rbair23 deleted the record-manager-v1 branch July 19, 2023 23:53
kimbor
kimbor approved these changes Jul 20, 2023
View reviewed changes
Copy link
Member

@kimbor kimbor left a comment

Choose a reason for hiding this comment

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

Filtering on files owned by me, LGTM!

vtronkov pushed a commit that referenced this pull request Jul 25, 2023
@jasperpotts @rbair23 @vtronkov
New Record Stream Facility (#6469)
0b246cc 
Signed-off-by: jasperpotts <jasperpotts@users.noreply.github.com>
Signed-off-by: Richard Bair <rbair23@users.noreply.github.com>
Co-authored-by: jasperpotts <jasperpotts@users.noreply.github.com>
Co-authored-by: Richard Bair <rbair23@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rbair23 rbair23 rbair23 approved these changes

@MiroslavGatsanoga MiroslavGatsanoga MiroslavGatsanoga left review comments

@mhess-swl mhess-swl mhess-swl left review comments

@david-bakin-sl david-bakin-sl david-bakin-sl left review comments

@kimbor kimbor kimbor approved these changes

@netopyr netopyr netopyr approved these changes

@nathanklick nathanklick nathanklick approved these changes

@Neeharika-Sompalli Neeharika-Sompalli Neeharika-Sompalli approved these changes

@iwsimon iwsimon iwsimon approved these changes

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

@povolev15 povolev15 Awaiting requested review from povolev15

Assignees

@jasperpotts jasperpotts

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement improved record handling implementation