Skip to content

Commit

Permalink
Small tweaks
Browse files Browse the repository at this point in the history
Besides naming conventions driver -> connector.  I replaced maybeVersion with a promise. 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.

A potential optimization would be to consider the backend healthy unless we already awaited for the promise version and failed. That can help with coldstarts if if being healthy is a precondition to send queries.
  • Loading branch information
miguelff committed Jul 26, 2023
1 parent 2a6ced8 commit ee86b21
Show file tree
Hide file tree
Showing 11 changed files with 47 additions and 125 deletions.
6 changes: 3 additions & 3 deletions query-engine/js-connectors/smoke-test-js/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ We assume Node.js `v18.16.1`+ is installed.
- Create a `.envrc` starting from `.envrc.example`, and fill in the missing values following the given template
- Install Node.js dependencies via
```bash
npm i
pnpm i
```

### PlanetScale
Expand All @@ -33,11 +33,11 @@ In the current directory:

## How to test

There is no automatic test. However, [./src/index.ts](./src/index.ts) includes a pipeline you can use to interactively experiment with the new Query Engine.
There is no automatic test. However, [./src/planetscale.ts](./src/planetscale.ts) includes a pipeline you can use to interactively experiment with the new Query Engine.

In particular, the pipeline steps are currently the following:

- Define `db`, a class instance wrapper around the `@planetscale/database` JS driver for PlanetScale
- Define `db`, a class instance wrapper around the `@planetscale/database` serverless driver for PlanetScale
- Define `nodejsFnCtx`, an object exposing (a)sync "Queryable" functions that can be safely passed to Rust, so that it can interact with `db`'s class methods
- Load the *debug* version of `libquery`, i.e., the compilation artifact of the `query-engine-node-api` crate
- Define `engine` via the `QueryEngine` constructor exposed by Rust
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ generator client {
}

datasource db {
// Once ran `prisma migrate reset` as per instructions in README.md replace `mysql` with `@prisma/planetscale` in the line below
provider = "@prisma/planetscale"
url = env("JS_PLANETSCALE_DATABASE_URL")
shadowDatabaseUrl = env("JS_PLANETSCALE_SHADOW_DATABASE_URL")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,16 +112,17 @@ type PlanetScaleConfig =

class PrismaPlanetScale implements Connector, Closeable {
private client: planetScale.Connection
private maybeVersion?: string
private versionPromise: Promise<string>
private isRunning: boolean = true

constructor(config: PlanetScaleConfig) {
this.client = planetScale.connect(config)

// lazily retrieve the version and store it into `maybeVersion`
this.client.execute('SELECT @@version, @@GLOBAL.version').then((results) => {
this.maybeVersion = 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));
});
}

async close(): Promise<void> {
Expand All @@ -133,10 +134,12 @@ class PrismaPlanetScale implements Connector, Closeable {
/**
* Returns false, if connection is considered to not be in a working state.
*/
isHealthy(): boolean {
const result = this.maybeVersion !== undefined
&& this.isRunning
return result
async isHealthy(): Promise<boolean> {
try {
return await this.versionPromise !== undefined && this.isRunning
} catch {
return false
}
}

/**
Expand Down Expand Up @@ -174,7 +177,7 @@ class PrismaPlanetScale implements Connector, Closeable {
* parsing or normalization.
*/
version(): Promise<string | undefined> {
return Promise.resolve(this.maybeVersion)
return this.versionPromise
}
}

Expand Down
13 changes: 13 additions & 0 deletions query-engine/js-connectors/smoke-test-js/src/connector/util.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@

import { Closeable, Connector } from '../engines/types/Library.js';

// *.bind(db) is required to preserve the `this` context.
// There are surely other ways than this to use class methods defined in JS within a
// connector context, but this is the most straightforward.
export const binder = (connector: Connector & Closeable): Connector & Closeable => ({
queryRaw: connector.queryRaw.bind(connector),
executeRaw: connector.executeRaw.bind(connector),
version: connector.version.bind(connector),
isHealthy: connector.isHealthy.bind(connector),
close: connector.close.bind(connector),
})
83 changes: 0 additions & 83 deletions query-engine/js-connectors/smoke-test-js/src/driver/mock.ts

This file was deleted.

13 changes: 0 additions & 13 deletions query-engine/js-connectors/smoke-test-js/src/driver/util.ts

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export type Connector = {
queryRaw: (params: Query) => Promise<ResultSet>
executeRaw: (params: Query) => Promise<number>
version: () => Promise<string | undefined>
isHealthy: () => boolean
isHealthy: () => Promise<boolean>
}

export type Closeable = {
Expand Down
13 changes: 7 additions & 6 deletions query-engine/js-connectors/smoke-test-js/src/planetscale.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@

import { setImmediate, setTimeout } from 'node:timers/promises'

import { binder } from './driver/util.js'
import { createPlanetScaleConnector } from './driver/planetscale.js'
import { binder } from './connector/util.js'
import { createPlanetScaleConnector } from './connector/planetscale.js'
import { initQueryEngine } from './util.js'

async function main() {
Expand All @@ -14,18 +14,19 @@ async function main() {
})

// `binder` is required to preserve the `this` context to the group of functions passed to libquery.
const driver = binder(db)
const conn = binder(db)

// wait for the database pool to be initialized
await setImmediate(0)

const engine = initQueryEngine(driver)
const engine = initQueryEngine(conn)

console.log('[nodejs] connecting...')
await engine.connect('trace')
console.log('[nodejs] connected')

console.log('[nodejs] isHealthy', await driver.isHealthy())
console.log('[nodejs] version', await conn.version())
console.log('[nodejs] isHealthy', await conn.isHealthy())

// Smoke test for PlanetScale that ensures we're able to decode every common data type.
const resultSet = await engine.query(`{
Expand Down Expand Up @@ -77,7 +78,7 @@ async function main() {

// Close the database connection. This is required to prevent the process from hanging.
console.log('[nodejs] closing database connection...')
await driver.close()
await conn.close()
console.log('[nodejs] closed database connection')

process.exit(0)
Expand Down
4 changes: 2 additions & 2 deletions query-engine/js-connectors/smoke-test-js/src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import fs from 'node:fs'

import { Connector, Library, QueryEngineInstance } from './engines/types/Library.js'

export function initQueryEngine(driver: Connector): QueryEngineInstance {
export function initQueryEngine(conn: Connector): QueryEngineInstance {
// I assume nobody will run this on Windows ¯\_(ツ)_/¯
const libExt = os.platform() === 'darwin' ? 'dylib' : 'so'
const dirname = path.dirname(new URL(import.meta.url).pathname)
Expand All @@ -29,7 +29,7 @@ export function initQueryEngine(driver: Connector): QueryEngineInstance {
}

const logCallback = (...args) => console.log(args)
const engine = new QueryEngine(queryEngineOptions, logCallback, driver)
const engine = new QueryEngine(queryEngineOptions, logCallback, conn)

return engine
}
12 changes: 6 additions & 6 deletions query-engine/js-connectors/src/proxy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,12 @@ pub struct Proxy {
}

/// Reify creates a Rust proxy to access the JS driver passed in as a parameter.
pub fn reify(js_driver: JsObject) -> napi::Result<Proxy> {
let query_raw = js_driver.get_named_property("queryRaw")?;
let execute_raw = js_driver.get_named_property("executeRaw")?;
let version = js_driver.get_named_property("version")?;
let close = js_driver.get_named_property("close")?;
let is_healthy = js_driver.get_named_property("isHealthy")?;
pub fn reify(js_connector: JsObject) -> napi::Result<Proxy> {
let query_raw = js_connector.get_named_property("queryRaw")?;
let execute_raw = js_connector.get_named_property("executeRaw")?;
let version = js_connector.get_named_property("version")?;
let close = js_connector.get_named_property("close")?;
let is_healthy = js_connector.get_named_property("isHealthy")?;

let driver = Proxy {
query_raw,
Expand Down
2 changes: 1 addition & 1 deletion query-engine/js-connectors/src/queryable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ impl QuaintQueryable for JsQueryable {

/// Returns false, if connection is considered to not be in a working state.
fn is_healthy(&self) -> bool {
// TODO: use self.driver.is_healthy()
// TODO: use self.proxy.is_healthy()
true
}

Expand Down

0 comments on commit ee86b21

Please sign in to comment.