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.

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

Switch to @libsql/client to support remote database connections #4

Merged
merged 10 commits into from
Nov 23, 2024

Conversation

solimaniac
Copy link
Contributor

Problem

When deploying the labeler server on container platforms with ephemeral file systems (e.g., Heroku, Railway), the SQLite database file would get deleted on each deploy since the filesystem is not persistent. This makes it difficult to maintain label history across deployments

Solution

The @libsql/client supports both local SQLite files and remote database connections. This PR updates the labeler server to:

  1. Replace the previous SQLite client with @libsql/client
  2. Add optional database connection parameters:
    • url - Remote database URL (e.g., "libsql://your-db.skyware.js.org")
    • authToken - Authentication token for remote database

Usage

// Local SQLite (existing behavior)
const server = new LabelerServer({
  did: "did:example:123",
  signingKey: "your-key",
  dbPath: "labels.db"
});

// Remote database
const server = new LabelerServer({
  did: "did:example:123",
  signingKey: "your-key",
  url: "libsql://your-db.skyware.js.org",
  authToken: "your-auth-token"
});

@solimaniac
Copy link
Contributor Author

Still need to test this with new & existing labelers to make sure it doesn't break anything

@solimaniac solimaniac mentioned this pull request Nov 11, 2024
@solimaniac solimaniac marked this pull request as ready for review November 12, 2024 03:58
@solimaniac
Copy link
Contributor Author

solimaniac commented Nov 12, 2024

Managed to do some testing locally, was able to verify backwards compatibility with reading and writing to .db files created with the old database client, as well as fresh setups of the labeler

Issues found while testing remote connections:

  • Turso instances fail when setting the journal_mode (seems to be a known issue with turso)
  • The logic for creating a labels table in the database is now asynchronous, which means that upon successfully defining a new LabelerServer an operation immediately following that that requires the table may fail due to a race condition

@futurGH I'm currently wresting with the idea of introducing an async InitializeServer function to deal with the latter issue, this would however be a 'breaking' change to server setup for folks looking to upgrade from older versions

@futurGH
Copy link
Contributor

futurGH commented Nov 14, 2024

Neat! Sorry I haven't had a chance to take a look at this yet, hoping to get to it this weekend.

  • The logic for creating a labels table in the database is now asynchronous, which means that upon successfully defining a new LabelerServer an operation immediately following that that requires the table may fail due to a race condition

I think the least breaking option here would be to introduce a dbLock promise as a class property that's awaited before anywhere the DB is accessed — not the cleanest solution but I don't like having "fake constructor" functions.

@solimaniac
Copy link
Contributor Author

I think the least breaking option here would be to introduce a dbLock promise as a class property that's awaited before anywhere the DB is accessed — not the cleanest solution but I don't like having "fake constructor" functions.

Agreed, done!

Also added a warning when the server fails to set the journal_mode for the database, I suspect not all remote servers would expose it by default (some providers like Turso may be handling this out of the box)

@futurGH
Copy link
Contributor

futurGH commented Nov 20, 2024

Finally getting a chance to take a look at this — is it ready for review?

@solimaniac
Copy link
Contributor Author

Finally getting a chance to take a look at this — is it ready for review?

Yep, should be ready for review! Working on a separate PR to update the documentation as well

@futurGH futurGH merged commit c7460ef into skyware-js:main Nov 23, 2024
@futurGH
Copy link
Contributor

futurGH commented Nov 23, 2024

Thanks! Will publish in 0.2.0.

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.

2 participants