Skip to content

Clean up hold_range_in_memory from accounts index: part 3#6920

Merged
HaoranYi merged 1 commit intoanza-xyz:masterfrom
HaoranYi:clean_hold_range2
Jul 16, 2025
Merged

Clean up hold_range_in_memory from accounts index: part 3#6920
HaoranYi merged 1 commit intoanza-xyz:masterfrom
HaoranYi:clean_hold_range2

Conversation

@HaoranYi
Copy link
Copy Markdown

@HaoranYi HaoranYi commented Jul 10, 2025

Problem

hold_range_in_memory was originally introduced to support rent scanning by keeping certain key ranges in memory. However, since we no longer perform range-based rent scanning, this mechanism is now obsolete.

This is part 3 for the clean up: refactor bucket_api item_in_range.

Summary of Changes

  • refactor bucket_api items_in_range to items
  • make items fn and BucketItems dev-context-util-only since they are now only used in tests.

Fixes #

@HaoranYi HaoranYi changed the title clean hold range2 Clean up hold_range_in_memory from accounts index. Jul 10, 2025
@HaoranYi HaoranYi marked this pull request as draft July 10, 2025 16:29
@HaoranYi HaoranYi force-pushed the clean_hold_range2 branch from 57335da to cc47227 Compare July 10, 2025 16:33
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.2%. Comparing base (5604510) to head (7586322).

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #6920     +/-   ##
=========================================
- Coverage    83.2%    83.2%   -0.1%     
=========================================
  Files         853      853             
  Lines      375491   375480     -11     
=========================================
- Hits       312567   312522     -45     
- Misses      62924    62958     +34     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@HaoranYi HaoranYi force-pushed the clean_hold_range2 branch 5 times, most recently from 55ce583 to 0118cad Compare July 11, 2025 21:37
R: RangeBounds<Pubkey>,
{
#[cfg(feature = "dev-context-only-utils")]
pub fn items(&self) -> Vec<BucketItem<T>> {
Copy link
Copy Markdown
Author

@HaoranYi HaoranYi Jul 12, 2025

Choose a reason for hiding this comment

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

Now this fn is only used in test. And we don't need to filter by range. so we can refactor it and put it in "dev-context-only-utils".

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It looks like theres one usage left? How hard would that be to remove?

Copy link
Copy Markdown
Author

@HaoranYi HaoranYi Jul 16, 2025

Choose a reason for hiding this comment

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

I think it is fine to keep it. It is only used in test and has been marked as "dev-context-only-utils". It won't be affecting validator binary.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

My thinking is more along the lines of:
If there is only one usage of this API and type, and it's in a test: Why is it needed at all?
Is the test in the end just in effect testing the API? Or if the test is testing something else, is there a better API it should be using.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ah. no, the test code, where it is used, is not testing this API. fn items() is a test utility API. It is used as a way to assert the content of bucketmap is expected in testing other APIs, such as assert, delete etc.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

thanks!

@HaoranYi HaoranYi force-pushed the clean_hold_range2 branch 2 times, most recently from d190442 to ccab019 Compare July 15, 2025 20:01
@HaoranYi HaoranYi force-pushed the clean_hold_range2 branch from ccab019 to 7586322 Compare July 16, 2025 14:11
@HaoranYi HaoranYi changed the title Clean up hold_range_in_memory from accounts index. Clean up hold_range_in_memory from accounts index: part 3 Jul 16, 2025
@HaoranYi HaoranYi marked this pull request as ready for review July 16, 2025 15:27
Copy link
Copy Markdown

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

:shipit:

@HaoranYi HaoranYi merged commit 0645f56 into anza-xyz:master Jul 16, 2025
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants