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

change(state): Set upper bound when reading from deleting column family tx_loc_by_transparent_addr_loc #7732

Merged
merged 9 commits into from
Oct 18, 2023
Merged

change(state): Set upper bound when reading from deleting column family tx_loc_by_transparent_addr_loc #7732

merged 9 commits into from
Oct 18, 2023

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Oct 11, 2023 •
edited
Loading

Motivation

This PR updates address_transaction_locations() to avoid using multiple iterators to read data from disk and to set an upper iterate bound.

This function is currently used by zebra-rpc for the get_address_tx_ids method.

Part of #7664.

Solution

  • Refactor address_transaction_locations() to use zs_range_iter instead of zs_next_key_value_from

Related cleanups:

  • Speed up rpc_getaddresstxids_response

Review

Anyone can review.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@arya2 arya2 added C-bug Category: This is a bug P-Medium ⚡ C-security Category: Security issues I-slow Problems with performance or responsiveness A-state Area: State / database changes C-tech-debt Category: Code maintainability issues I-remote-trigger Remote nodes can make Zebra do something bad labels Oct 11, 2023
@arya2 arya2 self-assigned this Oct 11, 2023
@arya2 arya2 requested a review from a team as a code owner October 11, 2023 22:43
@arya2 arya2 requested review from teor2345 and removed request for a team October 11, 2023 22:43
@arya2 arya2 marked this pull request as draft October 11, 2023 23:04
@arya2 arya2 marked this pull request as ready for review October 11, 2023 23:54
teor2345
teor2345 previously approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This is good except for an incorrect constant (which is currently technically valid due to a workaround in other code).

The names here could do with some changes to avoid confusion when we modify this code again, but that is optional.

@arya2 arya2 requested a review from teor2345 October 12, 2023 17:24
teor2345
teor2345 previously approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thank you! I think the changes are good, but the documentation is now outdated.

@teor2345
Copy link
Contributor

Can you also update the PR title? There aren't any read bounds being set in this PR, it's just a refactor that allows them to be set in the next PR.

It might also help to use the name of the column family or RPC, because there's just one of them.

@arya2 arya2 changed the title change(state): Set iterator read bounds on deleting column families change(state): Use zs_range_iter when reading from tx_loc_by_transparent_addr_loc column family Oct 12, 2023
@arya2 arya2 changed the title change(state): Use zs_range_iter when reading from tx_loc_by_transparent_addr_loc column family change(state): Set upper bound when reading from deleting column families Oct 12, 2023
@arya2 arya2 changed the title change(state): Set upper bound when reading from deleting column families change(state): Set upper bound when reading from deleting column family tx_loc_by_transparent_addr_loc Oct 13, 2023
@upbqdn upbqdn added the do-not-merge Tells Mergify not to merge this PR label Oct 13, 2023
@upbqdn upbqdn removed the do-not-merge Tells Mergify not to merge this PR label Oct 17, 2023
mergify bot added a commit that referenced this pull request Oct 18, 2023
@mergify
Merge of #7732
da6fff8
@mergify mergify bot mentioned this pull request Oct 18, 2023
Closed
@arya2
Copy link
Contributor Author

arya2 commented Oct 18, 2023

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Oct 18, 2023

refresh

✅ Pull request refreshed

mergify bot added a commit that referenced this pull request Oct 18, 2023
@mergify
Merge of #7732
2616e43
@mergify mergify bot mentioned this pull request Oct 18, 2023
Closed
@mergify mergify bot merged commit 01168c8 into main Oct 18, 2023
125 checks passed
@mergify mergify bot deleted the use-range-iter branch October 18, 2023 06:16
@mpguerra mpguerra linked an issue Oct 18, 2023 that may be closed by this pull request
Set iterator read bounds on deleting column families to avoid very slow syncs #7664
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 approved these changes

Assignees

@arya2 arya2

Labels
A-state Area: State / database changes C-bug Category: This is a bug C-security Category: Security issues C-tech-debt Category: Code maintainability issues I-remote-trigger Remote nodes can make Zebra do something bad I-slow Problems with performance or responsiveness
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Set iterator read bounds on deleting column families to avoid very slow syncs
3 participants
@arya2 @teor2345 @upbqdn