Small tweaks to smoke tests of planetscale driver in query-engines. #4085
Conversation
CodSpeed Performance ReportMerging #4085 will improve performances by 5.01%Comparing Summary
Benchmarks breakdown
|
miguelff
requested review from
a team,
SevInf and
aqrln
and removed request for
a team
The promise executes aynchrounsly on constructor, but it's awaited whenever the version is required, particularly to provide consistent health information. Otherwise isHealthy would be returning false, while the promise hasn't resolved yet.
|
I'm making further changes after neon support has been merged, will request review again once done. |
| provider = "@prisma/neon" | ||
| url = env("JS_NEON_DATABASE_URL") | ||
| shadowDatabaseUrl = env("JS_NEON_SHADOW_DATABASE_URL") | ||
| // Once ran `prisma migrate reset` as per instructions in README.md replace `mysql` with `@prisma/planetscale` in the line below |
There was a problem hiding this comment.
The comment implies that provider is mysql initially, should provider = "mysql" be committed instead, or should the comment be reworded to suggest replacing @prisma/planetscale with mysql before running prisma migrate reset?
| this.versionPromise = new Promise((resolve, reject) => { | ||
| this.pool.query('SELECT VERSION()') | ||
| .then((results) => resolve(results.rows[0]['version'])) | ||
| .catch((error) => reject(error)); |
There was a problem hiding this comment.
This catch will also run for errors that happen in the resolve callback and not just in this.pool.query, should this be the second argument to then instead?
Or better yet, could we replace the whole statement with
this.versionPromise = this.pool.query('SELECT VERSION()')
.then((results) => results.rows[0]['version'])`or is there a reason why we are wrapping it in another promise?
There was a problem hiding this comment.
No, I think we could define it the way you suggest, if resolve is a defined symbol within the closure.
There was a problem hiding this comment.
that was a copy-paste artifact, sorry, I meant
this.versionPromise = this.pool.query('SELECT VERSION()')
.then((results) => results.rows[0]['version'])`| this.versionPromise = new Promise((resolve, reject) => { | ||
| this.client.execute('SELECT @@version') | ||
| .then((results) => resolve(results.rows[0]['@@version'])) | ||
| .catch((error) => reject(error)); |
|
Note: I recommend deprecating this PR in favor of #4094. Please refer to the description and comments therein |
|
Closing in favor of #4094 |
* Renames and restructure * Use proxy in `JsQueryable::is_healthy` * Change neon and planetscale drivers to use a promise for the version The promise executes aynchrounsly on constructor, but it's awaited whenever the version is required, particularly to provide consistent health information. Otherwise isHealthy would be returning false, while the promise hasn't resolved yet. * feat: add child parent relation migration * feat(js-connectors): add support for transactions in PlanetScale, without touching Rust * chore: apply renames as in #4085 * chore: use switch case syntax * chore: add hard error if we detect intertwine transaction * feat(js-connectors): adopt Rust's "performIO" pattern to simplify isHealthy() and version() * feat(js-connectors): implement "is_healthy()" * feat(js-connectors): add data conversions for neon * feat(js-connector): add common ConnectorConfig for PlanetScale and Neon * feat(js-connector): update type mapping in neon, use common ConnectorConfig * feat(js-connector): use common ConnectorConfig * test(js-connector): split + centralise tests for PlanetScale and Neon, enable "@withFlavor" test decorator * chore: update README.md * chore: fix typo * chore: fix typo * chore: restore PlanetScale transactions * fix: address review comments * feat: simplify neon/planetscale data-type conversions --------- Co-authored-by: Miguel Fernandez <[email protected]>
Besides applying some naming conventions consistenly, I made changes to the connector to change the mechanics around initialisation and health checking.
In the planetscale serverless connector, I replaced
maybeVersionwith a promise. The query to retrieve the version executes aynchronously on the constructor as before, but it's awaited whenever the version is required, particularly to provide consistent health information. OtherwiseisHealthywould be returning false while the promise hasn't resolved yet.A potential optimization would be to consider the backend healthy until the promise is resolved and failed. That can help with coldstarts if being healthy is a precondition to send queries. Because we will be returning true for
isHealthyuntil proven otherwise by a query failing.