Skip to content

Conversation

@sfauvel
Copy link
Collaborator

@sfauvel sfauvel commented Oct 23, 2024

Content

This PR implement migration mechanism for the event_store module.
The event_store module is reorganized to embedded a database folder with the same structure than the mithril-aggregator::database one.
The WAL is activated on the event_store connection

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • CHANGELOG file is 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
  • Documentation
    • Update README file (if relevant)
    • Update documentation website (if relevant)
    • Add dev blog post (if relevant)

Comments

We do not create a folder for repository, record and query but just a file because there is only one structure in each one.
If we have to add other structure, we can create this directory later transparently for the other modules.

Issue(s)

Relates to #2023

@github-actions
Copy link

github-actions bot commented Oct 23, 2024

Test Results

    4 files  ±0     55 suites  ±0   10m 25s ⏱️ ±0s
1 397 tests +2  1 397 ✅ +2  0 💤 ±0  0 ❌ ±0 
1 611 runs  +2  1 611 ✅ +2  0 💤 ±0  0 ❌ ±0 

Results for commit 9a7bf92. ± Comparison against base commit 63e1204.

This pull request removes 3 and adds 5 tests. Note that renamed tests count towards both.
mithril-aggregator ‑ event_store::event::tests::can_persist_event
mithril-aggregator ‑ event_store::event::tests::event_projection
mithril-aggregator ‑ event_store::event::tests::provider_sql
mithril-aggregator ‑ event_store::database::query::tests::build_a_json_for_content_field_with_content_and_headers
mithril-aggregator ‑ event_store::database::query::tests::provider_sql
mithril-aggregator ‑ event_store::database::record::tests::event_projection
mithril-aggregator ‑ event_store::database::repository::tests::can_persist_event
mithril-aggregator ‑ event_store::database::repository::tests::migration_executed_running_database

♻️ This comment has been updated with latest results.

@sfauvel sfauvel changed the title Sfauvel/2023/refactor event store to use migration mechanism Refactor event store to use migration mechanism Oct 23, 2024
@sfauvel sfauvel force-pushed the sfauvel/2023/refactor_event_store_to_use_migration_mechanism branch from e9a8296 to d67e3fe Compare October 23, 2024 14:43
@sfauvel sfauvel temporarily deployed to testing-preview October 23, 2024 15:00 — with GitHub Actions Inactive
@sfauvel sfauvel temporarily deployed to testing-sanchonet October 23, 2024 15:00 — with GitHub Actions Inactive
@sfauvel sfauvel marked this pull request as ready for review October 23, 2024 15:00
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 👍

Copy link
Collaborator

@Alenar Alenar left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@dlachaume dlachaume left a comment

Choose a reason for hiding this comment

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

LGTM 👍

let mut rows = self.connection.fetch(InsertEventQuery::one(message)?)?;

rows.next().ok_or(anyhow!(
"No record from the database after I saved event message {log_message:?}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This error message was already present before, but I find the phrasing a bit strange with the use of 'I'. What do you think?

Suggested change
"No record from the database after I saved event message {log_message:?}"
"No record from the database after saving event message {log_message:?}"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@sfauvel sfauvel force-pushed the sfauvel/2023/refactor_event_store_to_use_migration_mechanism branch from 99fbdf1 to bb82041 Compare October 24, 2024 15:48
@sfauvel sfauvel temporarily deployed to testing-preview October 24, 2024 16:09 — with GitHub Actions Inactive
@sfauvel sfauvel temporarily deployed to testing-sanchonet October 24, 2024 16:09 — with GitHub Actions Inactive
@sfauvel sfauvel force-pushed the sfauvel/2023/refactor_event_store_to_use_migration_mechanism branch from 978854d to 9a7bf92 Compare October 25, 2024 07:43
@sfauvel sfauvel merged commit bb2b46b into main Oct 25, 2024
45 checks passed
@sfauvel sfauvel deleted the sfauvel/2023/refactor_event_store_to_use_migration_mechanism branch October 25, 2024 07:51
@sfauvel sfauvel temporarily deployed to testing-preview October 25, 2024 07:52 — with GitHub Actions Inactive
@sfauvel sfauvel temporarily deployed to testing-sanchonet October 25, 2024 07:52 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants