-
Notifications
You must be signed in to change notification settings - Fork 39
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
Upkeep service for aggregator & signer #1791
Conversation
Test Results 4 files ± 0 51 suites ±0 8m 51s ⏱️ -3s Results for commit 35138ad. ± Comparison against base commit 5816ed4. This pull request removes 1 and adds 14 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
fd6fc4e
to
bcb059a
Compare
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.
I'm wondering about what will happen during the cleanup for the REST API of the aggregator: will it be locked? Can we handle that situation with a 503
response?
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
@jpraynaud Currently if the repository layer fails because of an error (most likely a Returning another error code is possible: To do so we need to rework the What do you think ? |
I was thinking about a warp middleware that would return a |
bcb059a
to
c990901
Compare
As of now the only upkeep task is a cleanup of the databases by running a vacuum and/or a wal checkpoint to reduce their sizes.
* Allow to open in memory database only in test. * Avoid getting the same path from two different ways, with the only difference being the creation of the dir if it doesn't exist
And when the registration round is closed.
To not block the tokio runtime since tasks such as the vacuum can take some time.
By checking that the tasks are done using the logs. The actual check that the tasks do what they claims is done in the `SqliteCleaner` tests.
To make it use the same conventions as the aggregator. * Rename signer `database::test_utils` `database::test_helper` and move it to a dedicated file. * Copy the same `TestLogger` * Sync aggregator & signer `CardanoTransactionImporter`
Like the aggregator for now the only upkeep task is to cleanup the databases by running a vacuum and/or a wal checkpoint to reduce their sizes.
Executed at the start of the `unregistered > registered` state machine transition. Note: In the integration test the store have been changed from independent `MemoryAdapter` (that use hashmaps) to a `SQLiteAdapter` with a shared in memory connection to make it closer to the production environment.
* Since the UpkeepService now took this cleanup task. * Remove the `Vacuum` option from the ConnectionBuilder as it was the only use. * Remove `vacuum_database` function and instead use `SqliteCleaner` everywhere to streamline usage.
Since that would probably mean that some writing are currently occuring on the database (like if the cardano transactions preloader is running).
840941f
to
8fdd869
Compare
8fdd869
to
1bb16f1
Compare
03d6fbd
to
dec18c5
Compare
* Make signer run upkeep after registration * Make upkeep a separate task in aggregator runner * Make `Configuration::get_sqlite_dir` always create the directory if it doesn't exist instead of only in Production environment. As this behavior doesn't make tests crash anymore now. * Typo fix
This method will replace replace most usage of the `reply::internal_server_error` as it analyse as it can change the http error code returned based on the error given: * by default the error is `500 - InternalServerError` * For Sqlite Busy errors or RegistrationRoundNotYetOpened error the error code change to `503 - ServiceUnavailable`
Mostly replacing usage of `reply::internal_server_error` as the new function can return varius status code depending on the error.
As it's used as the reply message for multiple server side errors, not just `500 - InternalServerError`.
* Mithril-aggregator from `0.5.32` to `0.5.33` * Mithril-signer from `0.2.155` to `0.2.156` * Mithril-common from `0.4.24` to `0.4.25` * Mithril-persistence from `0.2.13` to `0.2.14`
dec18c5
to
35138ad
Compare
Content
This PR add a new service to
mithril-aggregator
andmithril-signer
, theUpkeepService
:Idle → Ready
state machine transition, while the registration round is close.Unregistered → Registered
state machine transition.pragma wal_checkpoint(TRUNCATE)
on both the main and the cardano transaction database to avoid infinite grow of the wal files.Note: the vacuum is not run on the cardano transaction database as it can be so large that it takes more than an hour on the mainnet.
Pre-submit checklist
Issue(s)
Closes #1707