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

Fixing slow_test store.pruned_load. #3663

Merged
merged 2 commits into from
Jan 13, 2022
Merged

Conversation

clemahieu
Copy link
Contributor

This was crashing on the line store->pruned.del as hashes was empty and querying for .begin (). hashes was never populated even when this test was originally added.

…re->pruned.del as hashes was empty and querying for .begin (). hashes was never populated even when this test was originally added.
@clemahieu clemahieu requested a review from dsiganos January 11, 2022 14:52
}
}
if (!nano::rocksdb_config::using_rocksdb_in_tests ())
{
auto transaction (store->tx_begin_write ());
for (auto k (0); k < batch_size / 2; ++k)
for (auto k (0); !hashes.empty () && k < batch_size / 2; ++k)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the check !hashes.empty ()?
This is a unit test, if anything is wrong, we want to fail loud and clear rather than handle it quietly.
So I would say that we should ASSERT that hashes is not empty.

@zhyatt zhyatt added the unit test Related to a new, changed or fixed unit test label Jan 11, 2022
@zhyatt zhyatt added this to the V24.0 milestone Jan 11, 2022
dsiganos
dsiganos previously approved these changes Jan 12, 2022
…databases (#3666)

Previously, it did one thing for lmdb and another for rocksdb due to
rocksdb not having an accurate count of blocks.
The block counting is now done manually by iterating through all the blocks.
@dsiganos dsiganos merged commit 2752cfe into develop Jan 13, 2022
@dsiganos dsiganos deleted the cl_store_pruned_load_fix branch January 13, 2022 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unit test Related to a new, changed or fixed unit test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants