feat(sqlite): Add an fx module for sqlite #223
Conversation
imiric
left a comment
imiric
left a comment
There was a problem hiding this comment.
Very nice, thank you! 🙇
I'm still getting used to the fx way of doing things, so most of my feedback is related to that. The rest are nitpicks and minor suggestions.
I appreciate the example, and being able to run migrations on startup. 👍
I'll integrate this in firewall as soon as it's merged.
wdullaer
force-pushed
the
push-vukxpqozslzz
branch
4 times, most recently
from
fe435b0 to
9cc6bc5
Compare
|
@wdullaer Is there anything pending here, or can it be merged? We'd like to integrate this in firewall, though it's not urgent. |
Fumesover
left a comment
Fumesover
left a comment
There was a problem hiding this comment.
Any reason to diverge from the fxXXXX naming scheme for modules? I would have expected fxsqlite instead of just sqlite
| if sqliteConf.busyTimeout != time.Duration(0) { | ||
| conn.SetBusyTimeout(sqliteConf.busyTimeout) | ||
| } | ||
| return sqlitex.ExecuteTransient(conn, "PRAGMA foreign_keys = ON;", &sqlitex.ExecOptions{}) |
There was a problem hiding this comment.
Is this used anywhere in our repositories? I'm not seeing this in the pidx creation for blobd
There was a problem hiding this comment.
It's used in firewall.
Sqlite disables this by default for backwards compatibility. It's kind of hidden footgun these modules are intended to prevent.
There was a problem hiding this comment.
I'm not sure this attempt at homogenizing our sqlite practice is the right one. Mostly because I don't think this is the correct driver/API that we want to expose for the next project that uses sqlite.
Let's recap what we use atm at exoscale:
- Blobd uses
zombiezen.com/go/sqlite/sqlitexbecause we need very high speed sqlite operation and thus can't use the slowerdatabase/sqlAPI (we need control on memory allocation) - I think firewall-agent also uses
zombiezen.com/go/sqlite/sqlitex, but why chose this one instead of the regularmattn/sqlite3driver that is API compatible with thedatabase/sqlinterface - Nbdagent uses
mattn/sqlite3to get adatabse/sql-compatible driver and then usesjmoiron/sqlxas the query lib - compute-agent uses
mattn/sqlite3to get adatabse/sql-compatible driver and then usesgormas the query lib - blobd-orchestrator uses
mattn/sqlite3to get adatabse/sql-compatible driver and then usesgormas the query lib - warp uses
mattn/sqlite3to get adatabse/sql-compatible driver and then usesgormas the query lib
Blobd-orchestrator & compute-agent will not be able to use this without major rewrite (or never at all for blobd-orchestrator because we use gorm to abstract sqlite/mysql differences)
Maybe we should rather
- Make stelling provide a
database/sql-compatible dirver - Change firewall-agent to use this API
- Converge on this expect for blobd that has very specific performance characteristics ?
WDYT ?
|
I think the list here shows that some standardisation is needed. I suppose the way forward is the approach we took with the migration library: we support both. I'll update this PR to move the existing code into a |
|
That's fine for me, and let's clearly document from a glance that the |
wdullaer
force-pushed
the
push-vukxpqozslzz
branch
from
9cc6bc5 to
c8158ac
Compare
Sqlite has become widely used in our ecosystem. This module moves the common boilerplate and best practices into a central place.
wdullaer
force-pushed
the
push-vukxpqozslzz
branch
from
c8158ac to
66c6576
Compare
|
Since this hasn't merged yet, I added the |
Additionally the module also provides a gorm.DB if needed. The module API is identical to the sqlitex one, to make it easier to swap out if the requirements change and require the higher performing driver.
wdullaer
force-pushed
the
push-vukxpqozslzz
branch
from
1ad37ec to
7ac8eaa
Compare
6 participants
Sqlite has become widely used in our ecosystem.
This module moves the common boilerplate and best practices into a central place.