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

Add separate config to disable balance pruning #354

Closed
wants to merge 2 commits into from

Conversation

sink772
Copy link

@sink772 sink772 commented Sep 14, 2022

Motivation

We are experiencing a similar performance issue described in #316, when we are running check:data command.

We found that PruneBalances operation is the root culprit of the performance issue.
As you know, by default, the PruneBalances is called whenever a balance change happened for an account in a block.
BTW, in some blockchains (like ICON), there is an account that is always included in every block (eg, fee_treasury), which makes only one reconcile goroutine is effectively active and the other goroutines that call reconcileActiveAccounts method would block on rosetta-sdk-go/storage/database.(*BadgerDatabase).WriteTransaction.

Solution

There is an existing config for disabling pruning, PruningDisabled. But this config also applies to the state storage pruning.
We want to introduce a separate config only for balance pruning operation, named PruningBalanceDisabled.

Open questions

None

@@ -153,7 +153,7 @@ func (h *ReconcilerHelper) PruneBalances(
currency *types.Currency,
index int64,
) error {
if h.config.Data.PruningDisabled {
if h.config.Data.PruningDisabled || h.config.Data.PruningBalanceDisabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can fully decouple pruning, so only h.config.Data.PruningBalanceDisabled is needed here

Copy link
Author

Choose a reason for hiding this comment

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

I fixed it that way to respect the intent of the original author.
Okay, I will fix the code so that only PruningBalanceDisabled is needed. I also think that is better.

@coinbase coinbase deleted a comment from cb-heimdall Sep 14, 2022
@GeekArthur
Copy link
Contributor

@sink772 Please also fix the CI test for introducing new config

@sink772 sink772 requested a review from GeekArthur September 19, 2022 07:41
@GeekArthur
Copy link
Contributor

@sink772 Approved, the commits need to be signed for merging

@GeekArthur
Copy link
Contributor

@sink772 FYI, I merged another PR for decoupling pruning configurations with new names: #362

I close this PR as that PR includes the changes of this PR, thanks for your work!

@GeekArthur GeekArthur closed this Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants