-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Refactors AccountsIndex::get() #35163
Refactors AccountsIndex::get() #35163
Conversation
None => AccountIndexGetResult::NotFound, | ||
} | ||
} | ||
None => AccountIndexGetResult::NotFound, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to have this NotFound
be duplicated.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #35163 +/- ##
=========================================
- Coverage 81.6% 81.6% -0.1%
=========================================
Files 832 832
Lines 224891 224887 -4
=========================================
- Hits 183612 183606 -6
- Misses 41279 41281 +2 |
} | ||
None => AccountIndexGetResult::NotFound, | ||
} | ||
self.get_account_read_entry(pubkey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we want to get rid of ReadAccountMapEntry
I think we may get rid of all calls to get
#34918
I think we may still call get
for secondary index scan (which should never be used, iirc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm working on that part too. Here was a small part that I broke off the whole work-in-progress. It was helpful for me to remove the code that was in get()
that duplicates the code in get_account_read_entry()
. I can stash all this and only do the changes that remove ReadAccountMapEntry
too.
accounts-db/src/accounts_index.rs
Outdated
None => AccountIndexGetResult::NotFound, | ||
} | ||
self.get_account_read_entry(pubkey) | ||
.and_then(|entry| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you rename entry
to locked_entry
?
This will make the diff line up and me less nervous ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 80da19a.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
I like that we share the code and get rid of one direct use of ReadAccountMapEntry::from_account_map_entry
. In future, when we get rid of the self-ref struct, one less place to change!
Yep, that's the plan. I'm working my way through removing the struct. Still some more to go though. |
Problem
The impl of
AccountsIndex::get()
contains code duplication, and is a bit verbose.Summary of Changes
Refactor to remove code duplication and be more direct.