- 
                Notifications
    You must be signed in to change notification settings 
- Fork 302
feat(fortuna): Support Postgres DB backend #2841
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
Conversation
| The latest updates on your projects. Learn more about Vercel for Git ↗︎ 
 | 
…b/fortuna/postgres
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 but please address the inline comments
| pub async fn new() -> Result<Self> { | ||
| Self::new_with_url("sqlite:fortuna.db?mode=rwc").await | ||
| let database_url = | ||
| std::env::var("DATABASE_URL").unwrap_or_else(|_| DEFAULT_DATABASE_URL.to_string()); | 
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.
can this be a config value? it's confusing to have configuration both via a file and via env. I can see this being useful for secrets or something, but this doesn't seem particularly sensitive.
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.
The sqlx tool uses dotenv to read the URL from the .env file, so we use that in Fortuna as well (we could load it into the env in a different way if needed in k8s.) It helps devex IMO to use the same .env file in Fortuna that sqlx uses, as sometimes it's necessary to setup/recover migrations using the tool. Also the connection string has credentials in it, so better to consider it a secret rather than a config
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 hear you here but it's very confusing to have two ways of passing configs. i think moving this to run.rs might be better and regardless i recommend logging this (as info level) that you are using this data base url to help people catch it if they don't set it.
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.
Yeah fair enough, it can be confusing to have 2 config sources. Since we are ok with exposing the DB credentials, I'll go ahead and unify to config.yaml.
        
          
                apps/fortuna/src/history.rs
              
                Outdated
          
        
      | .max_lifetime(None) | ||
| .connect("sqlite::memory:") | ||
| .await?; | ||
| let migrator = migrate!("./migrations"); | 
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.
is the sqlite_migrations directory used anywhere? if not, delete please
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.
(also suggest extracting a string constant for the migrations directory)
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.
Ah yep forgot to delete, thanks
| "chrono", | ||
| ] } | ||
| num-traits = "0.2.19" | ||
| dotenv = "0.15.0" | 
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.
this seems abandoned (from sqlx docs), can you use dotenvy?
Summary
Support both Postgres and SQLite database backends instead of just SQLite. This lets us use a hosted Postgres database that multiple replicas can connect to, instead of a file-based one. We still use an in-memory SQLite for the History tests.
Rationale
A central database shared between the replicas gives us a consistent storage to serve the API from.
Some impl details:
AnyPoolsqlx driver instead of aPool<Sqlite>. It chooses the correct driver at runtime based on the URL passed in.explorer.rs, similar to the update queries inhistory.rs.How has this been tested?
Tested Fortuna's
/v1/logs(explorer) API with a SQLite backend and a cloud Postgres backend from Neon. Also tested with multiple replicas pointing to the same DB, which is our intended production configuration.