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

fix: LongList read from file #16994

Conversation

thenswan
Copy link
Contributor

@thenswan thenswan commented Dec 9, 2024

Description:
This PR introduces various fixes and improvements to the Long List family and its tests:

  • LongListHeap fixes:

    • Resolved an issue where snapshots failed if the valid range did not start at index 0.
  • AbstractLongList fixes:

    • Addressed a problem where writing and then reading an empty snapshot failed.
    • Addressed an issue where loading an empty Long List with a previously set valid range (e.g., 0 to a specific value) caused incorrect initialization of minValidIndex, maxValidIndex, and size.
  • Refactoring AbstractLongList (and its subclasses):

    • Moved chunk orchestration logic into readBodyFromFileChannelOnInit() and delegated chunk-specific reading to a new readChunkData() method in subclasses.
    • Extracted repetitive code.
  • Improvements to AbstractLongListTest (and its children):

    • Added tests to cover more edge cases.
    • Fixed existing tests (e.g., AbstractLongListTest#writeReadEmptyList() had a bug).
    • Consolidated LongList tests into AbstractLongListTest (migrated them from child classes).
    • Ensured snapshots from one implementation (e.g., LongListHeap) can be loaded by another (e.g., LongListOffHeap).
    • Renamed LongListTest to LongListAdHocTest; moved some tests into AbstractLongListTest.
    • Improved code structure (renamed methods/variables, added inline comments, etc.).

Notes for reviewers:

  • Skipped using LongListDisk#resetTransferBuffer() in the refactored AbstractLongListTest as it seems unnecessary.
  • Possibly convert ordered tests in AbstractLongListTest to regular tests.
  • Please resolve any outdated comments. 🙏

Related issue(s):
Fixes #16652

Checklist

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

Signed-off-by: Nikita Lebedev <nikita.lebedev@limechain.tech>
@thenswan thenswan self-assigned this Dec 9, 2024
@thenswan thenswan added this to the v0.58 milestone Dec 9, 2024
Copy link

codecov bot commented Dec 9, 2024

Codecov Report

Attention: Patch coverage is 92.53731% with 5 lines in your changes missing coverage. Please review.

Project coverage is 68.95%. Comparing base (829e18c) to head (49de0c1).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
...swirlds/merkledb/collections/AbstractLongList.java 91.17% 1 Missing and 2 partials ⚠️
...com/swirlds/merkledb/collections/LongListDisk.java 88.23% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main   #16994      +/-   ##
============================================
- Coverage     68.96%   68.95%   -0.01%     
- Complexity    22763    22770       +7     
============================================
  Files          2619     2619              
  Lines         98283    98288       +5     
  Branches      10185    10185              
============================================
+ Hits          67777    67779       +2     
- Misses        26675    26677       +2     
- Partials       3831     3832       +1     
Files with missing lines Coverage Δ
...com/swirlds/merkledb/collections/LongListHeap.java 89.13% <100.00%> (-8.75%) ⬇️
.../swirlds/merkledb/collections/LongListOffHeap.java 91.80% <100.00%> (-1.35%) ⬇️
...com/swirlds/merkledb/collections/LongListDisk.java 84.14% <88.23%> (-1.17%) ⬇️
...swirlds/merkledb/collections/AbstractLongList.java 88.51% <91.17%> (+0.27%) ⬆️

... and 8 files with indirect coverage changes

Impacted file tree graph

Copy link

codacy-production bot commented Dec 9, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.00% (target: -1.00%) 97.01%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (829e18c) 98066 71457 72.87%
Head commit (49de0c1) 98071 (+5) 71460 (+3) 72.87% (+0.00%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#16994) 67 65 97.01%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Signed-off-by: Nikita Lebedev <nikita.lebedev@limechain.tech>
…f-its-valid-range-does-not-start-from-0

# Conflicts:
#	platform-sdk/swirlds-merkledb/src/main/java/com/swirlds/merkledb/collections/LongListDisk.java
Signed-off-by: Nikita Lebedev <nikita.lebedev@limechain.tech>
@thenswan thenswan force-pushed the 16652-longlistheap-snapshots-are-broken-if-its-valid-range-does-not-start-from-0 branch from 676dd90 to 089bafd Compare December 11, 2024 12:08
@thenswan thenswan marked this pull request as ready for review December 11, 2024 14:22
@thenswan thenswan requested review from a team as code owners December 11, 2024 14:22
@thenswan thenswan requested a review from rbair23 December 11, 2024 14:22
imalygin
imalygin previously approved these changes Dec 11, 2024
Signed-off-by: Nikita Lebedev <nikita.lebedev@limechain.tech>
Signed-off-by: Nikita Lebedev <nikita.lebedev@limechain.tech>
Signed-off-by: Nikita Lebedev <nikita.lebedev@limechain.tech>
Signed-off-by: Nikita Lebedev <nikita.lebedev@limechain.tech>
Signed-off-by: Nikita Lebedev <nikita.lebedev@limechain.tech>
Signed-off-by: Nikita Lebedev <nikita.lebedev@limechain.tech>
Signed-off-by: Nikita Lebedev <nikita.lebedev@limechain.tech>
Signed-off-by: Nikita Lebedev <nikita.lebedev@limechain.tech>
@thenswan thenswan marked this pull request as ready for review January 17, 2025 11:34
Signed-off-by: Nikita Lebedev <nikita.lebedev@limechain.tech>
Signed-off-by: Nikita Lebedev <nikita.lebedev@limechain.tech>
imalygin
imalygin previously approved these changes Jan 17, 2025
Signed-off-by: Nikita Lebedev <nikita.lebedev@limechain.tech>
imalygin
imalygin previously approved these changes Jan 21, 2025
artemananiev
artemananiev previously approved these changes Jan 22, 2025
Signed-off-by: Nikita Lebedev <nikita.lebedev@limechain.tech>
@thenswan thenswan dismissed stale reviews from artemananiev and imalygin via 49de0c1 January 23, 2025 07:33
@thenswan thenswan merged commit 2b2c647 into main Jan 23, 2025
50 of 52 checks passed
@thenswan thenswan deleted the 16652-longlistheap-snapshots-are-broken-if-its-valid-range-does-not-start-from-0 branch January 23, 2025 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LongListHeap snapshots are broken if its valid range does not start from 0
3 participants