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

Remove legacy store adapters & fix prune of stake pool & epoch settings #1076

Merged
merged 8 commits into from
Jul 26, 2023
Merged

Remove legacy store adapters & fix prune of stake pool & epoch settings #1076

merged 8 commits into from
Jul 26, 2023

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Jul 21, 2023 •
edited
Loading

Content

This PR remove the StoreAdapter implementation that were remaining in the aggregator database providers. Instead of using them in the dependencies it's the trait or even the provider itself that are used now.
This simplify the usage of those database providers.

Also, this PR fix the prune of the Stake Pool & Epoch Settings, their queries were prepared but not executed since their cursors where not consumed. Tests have been added in order to check those cases.

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

Issue(s)

Closes #1053

@github-actions
Copy link

github-actions bot commented Jul 21, 2023 •
edited
Loading

Test Results

    3 files  ±0    16 suites  ±0   6m 48s ⏱️ +28s
640 tests  - 5  640 ✔️  - 5  0 💤 ±0  0 ❌ ±0 
678 runs   - 5  678 ✔️  - 5  0 💤 ±0  0 ❌ ±0 

Results for commit ef9c487. ± Comparison against base commit 5ea211e.

This pull request removes 10 and adds 5 tests. Note that renamed tests count towards both. 
mithril-aggregator ‑ database::provider::certificate::tests::test_store_adapter
mithril-aggregator ‑ database::provider::epoch_setting::tests::test_store_adapter
mithril-aggregator ‑ store::certificate_store::test::get_certificate_with_good_hash
mithril-aggregator ‑ store::certificate_store::test::get_certificate_with_wrong_hash
mithril-aggregator ‑ store::certificate_store::test::get_from_beacon
mithril-aggregator ‑ store::certificate_store::test::get_from_beacon_not_exist
mithril-aggregator ‑ store::certificate_store::test::list_has_some_members
mithril-aggregator ‑ store::certificate_store::test::list_is_empty
mithril-aggregator ‑ store::certificate_store::test::save_certificate_once
mithril-aggregator ‑ store::certificate_store::test::update_certificate

mithril-aggregator ‑ configuration::test::safe_epoch_retention_limit_wont_change_a_none_value
mithril-aggregator ‑ configuration::test::safe_epoch_retention_limit_wont_change_a_value_higher_than_three
mithril-aggregator ‑ configuration::test::safe_epoch_retention_limit_wont_yield_a_value_lower_than_three
mithril-aggregator ‑ database::provider::epoch_setting::tests::save_protocol_parameters_prune_older_epoch_settings
mithril-aggregator ‑ database::provider::stake_pool::tests::save_protocol_parameters_prune_older_epoch_settings

♻️ This comment has been updated with latest results.

@Alenar Alenar force-pushed the djo/1053/remove-legacy-store-adapters branch 3 times, most recently from 93ed08b to 951c32d Compare July 21, 2023 15:17
@Alenar Alenar temporarily deployed to testing-preview July 21, 2023 16:42 — with GitHub Actions Inactive
@Alenar Alenar force-pushed the djo/1053/remove-legacy-store-adapters branch 2 times, most recently from 2f4c0d4 to 946a895 Compare July 21, 2023 16:50
@Alenar Alenar temporarily deployed to testing-preview July 21, 2023 16:58 — with GitHub Actions Inactive
jpraynaud
jpraynaud approved these changes Jul 24, 2023
View reviewed changes
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Except for the pruning of the stores in the end to end test by default, and few typos/questions

mithril-aggregator/src/database/provider/stake_pool.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/services/stake_distribution.rs Show resolved Hide resolved
mithril-test-lab/mithril-end-to-end/src/mithril/aggregator.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/dependency_injection/builder.rs Outdated Show resolved Hide resolved
@Alenar Alenar force-pushed the djo/1053/remove-legacy-store-adapters branch 2 times, most recently from cc17d90 to d31de78 Compare July 25, 2023 08:26
@Alenar Alenar temporarily deployed to testing-preview July 25, 2023 08:33 — with GitHub Actions Inactive
@Alenar
Use directlyCertificateRepository instead of the CertificateStore
fd252ca
@Alenar
Use directly EpochSettingStore instead of the ProtocolParameterStore
a7f27b3
@Alenar
Fix prune when saving for EpochSettingStore
e19e019 
It did not execute at all since the cursor wasn't consumed.
@Alenar
Fix prune when saving for StakePoolStore
a7a0711 
It did not execute at all since the cursor wasn't consumed.
@Alenar
Increase STAKE_POOL_PRUNE_EPOCH_THRESHOLD to 10
eaeee84 
In order to have some breath if something goes wrong while pruning
enough data to keep that table lean.
@Alenar
Use retention limit from configuration instead of constants
8f555da 
This allow to synchronise pruning between tables, else pruning of data
can occurs from tables that is used as a foreign key in another.
Also, enable data pruning in the e2e tests for the aggregator in order
to test it.
@Alenar
Add safe_epoch_retention_limit to Configuration
cbfe804 
In order to ensure that if a retention limit is set it's not lower than
a minimum value in order to not prune data that will be used at later
epochs.
@Alenar
Update versions
ef9c487
@Alenar Alenar force-pushed the djo/1053/remove-legacy-store-adapters branch from d31de78 to ef9c487 Compare July 26, 2023 09:15
@Alenar Alenar temporarily deployed to testing-preview July 26, 2023 09:37 — with GitHub Actions Inactive
@Alenar Alenar merged commit 456a5ae into main Jul 26, 2023
26 checks passed
@Alenar Alenar deleted the djo/1053/remove-legacy-store-adapters branch July 26, 2023 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpraynaud jpraynaud jpraynaud approved these changes

@ghubertpalo ghubertpalo ghubertpalo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove legacy store adapters from aggregator
3 participants
@Alenar @jpraynaud @ghubertpalo