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

feat: Improved connection pool for HANAService #349

Merged
merged 15 commits into from
Dec 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion db-service/lib/common/DatabaseService.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ class DatabaseService extends cds.Service {

// Acquire a pooled connection
this.dbc = await this.acquire()
this.dbc.destroy = this.destroy.bind(this)

// Begin a session...
try {
Expand Down Expand Up @@ -110,8 +111,20 @@ class DatabaseService extends cds.Service {
*/
async release() {
if (!this.dbc) return
await this.pool.release(this.dbc)
const dbc = this.dbc
this.dbc = undefined
await this.pool.release(dbc)
}

/**
* Destroys own connection, i.e. tix.dbc, from this.pool
* This is for subclasses to intercept, if required.
*/
async destroy() {
if (!this.dbc) return
const dbc = this.dbc
this.dbc = undefined
await this.pool.destroy(dbc)
}

// REVISIT: should happen automatically after a configurable time
Expand Down
51 changes: 38 additions & 13 deletions hana/lib/HANAService.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,35 @@ class HANAService extends SQLService {

// REVISIT: Add multi tenant factory when clarified
get factory() {
const driver = drivers[this.options.driver || this.options.credentials.driver]?.driver || drivers.default.driver
const isMultitenant = 'multiTenant' in this.options ? this.options.multiTenant : cds.env.requires.multitenancy
const service = this
return {
options: { max: 1, ...this.options.pool },
create: async (/*tenant*/) => {
const driver = drivers[this.options.driver || this.options.credentials.driver]?.driver || drivers.default.driver
const dbc = new driver(this.options.credentials)
await dbc.connect()
HANAVERSION = dbc.server.major
return dbc
options: {
min: 0,
max: 10,
acquireTimeoutMillis: cds.env.profiles.includes('production') ? 1000 : 10000,
idleTimeoutMillis: 60000,
evictionRunIntervalMillis: 100000,
numTestsPerEvictionRun: Math.ceil((this.options.pool?.max || 10) - (this.options.pool?.min || 0) / 3),
...(this.options.pool || {}),
testOnBorrow: true,
fifo: false
},
create: async function (tenant) {
try {
const creds = isMultitenant
? await require('@sap/cds-mtxs/lib').xt.serviceManager.get(tenant, { disableCache: false })
: service.options.credentials
const dbc = new driver(creds)
await dbc.connect()
HANAVERSION = dbc.server.major
return dbc
} catch (err) {
if (!isMultitenant || err.code !== 10) throw err
await require('@sap/cds-mtxs/lib').xt.serviceManager.get(tenant, { disableCache: true })
return this.create(tenant)
}
},
error: (err /*, tenant*/) => {
// Check whether the connection error was an authentication error
Expand All @@ -52,7 +73,7 @@ class HANAService extends SQLService {
}
},
destroy: dbc => dbc.disconnect(),
validate: (/*dbc*/) => true,
validate: (dbc) => dbc.validate(),
}
}

Expand All @@ -63,14 +84,18 @@ class HANAService extends SQLService {
return `hana@${host}:${port}${driver ? `(${driver})` : ''}`
}

ensureDBC() {
return this.dbc || cds.error`Database is disconnected`
}

async set(variables) {
// REVISIT: required to be compatible with generated views
if (variables['$valid.from']) variables['VALID-FROM'] = variables['$valid.from']
if (variables['$valid.to']) variables['VALID-TO'] = variables['$valid.to']
if (variables['$user.id']) variables['APPLICATIONUSER'] = variables['$user.id']
if (variables['$user.locale']) variables['LOCALE'] = variables['$user.locale']

this.dbc.set(variables)
this.ensureDBC().set(variables)
}

async onSELECT({ query, data }) {
Expand Down Expand Up @@ -200,11 +225,11 @@ class HANAService extends SQLService {

// prepare and exec are both implemented inside the drivers
prepare(sql) {
return this.dbc.prepare(sql)
return this.ensureDBC().prepare(sql)
}

exec(sql) {
return this.dbc.exec(sql)
return this.ensureDBC().exec(sql)
}

/**
Expand Down Expand Up @@ -917,12 +942,12 @@ class HANAService extends SQLService {

onCOMMIT() {
DEBUG?.('COMMIT')
return this.dbc.commit()
return this.dbc?.commit()
}

onROLLBACK() {
DEBUG?.('ROLLBACK')
return this.dbc.rollback()
return this.dbc?.rollback()
}

// Creates a new database using HDI container groups
Expand Down
10 changes: 9 additions & 1 deletion hana/lib/drivers/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ class HANADriver {

set(variables) {
variables
throw new Error('Implementation missing')
throw new Error('Implementation missing "set"')
}

/**
Expand Down Expand Up @@ -139,6 +139,14 @@ class HANADriver {
return prom(this._native, 'disconnect')()
}
}

/**
* Validates that the connection is connected
* @returns {Promise<Boolean>}
*/
async validate() {
throw new Error('Implementation missing "validate"')
}
}

/**
Expand Down
37 changes: 35 additions & 2 deletions hana/lib/drivers/hana-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,41 @@ const { driver, prom, handleLevel } = require('./base')

const streamUnsafe = false

const credentialMappings = [
{ old: 'schema', new: 'currentSchema' },
{ old: 'certificate', new: 'ca' },
{ old: 'encrypt', new: 'useTLS' },
{ old: 'hostname_in_certificate', new: 'sslHostNameInCertificate' },
{ old: 'validate_certificate', new: 'sslValidateCertificate' },
]

class HANAClientDriver extends driver {
/**
* Instantiates the HANAClientDriver class
* @param {import('./base').Credentials} creds The credentials for the HANAClientDriver instance
*/
constructor(creds) {
// REVISIT: make sure to map all credential properties for hana-client
creds.currentSchema = creds.schema
// Enable native @sap/hana-client connection pooling
creds = Object.assign({}, creds, {
// REVISIT: add pooling related credentials when switching to native pools
// Enables the @sap/hana-client native connection pool implementation
// pooling: true,
// poolingCheck: true,
// maxPoolSize: 100, // TODO: align to options.pool configurations

// If communicationTimeout is not set queries will hang for over 10 minutes
communicationTimeout: 60000,
// connectTimeout: 1000,
// compress: true, // TODO: test whether having compression enabled makes things faster
// statement caches come with a side effect when the database structure changes which does not apply to CAP
// statementCacheSize: 100, // TODO: test whether statementCaches make things faster
})

// Retain node-hdb credential mappings to @sap/hana-client credential mapping
for (const m of credentialMappings) {
if (m.old in creds && !(m.new in creds)) creds[m.new] = creds[m.old]
}

super(creds)
this._native = hdb.createConnection(creds)
this._native.setAutoCommit(false)
Expand Down Expand Up @@ -55,6 +82,10 @@ class HANAClientDriver extends driver {
return ret
}

async validate() {
return this._native.state() === 'connected'
}

_extractStreams(values) {
// Removes all streams from the values and replaces them with a placeholder
if (!Array.isArray(values)) return { values: [], streams: [] }
Expand Down Expand Up @@ -88,6 +119,8 @@ class HANAClientDriver extends driver {
}
}

HANAClientDriver.pool = true

async function* rsIterator(rs, one) {
const next = prom(rs, 'next') // () => rs.next()
const getValue = prom(rs, 'getValue') // nr => rs.getValue(nr)
Expand Down
5 changes: 5 additions & 0 deletions hana/lib/drivers/hdb.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ class HDBDriver extends driver {
super(creds)
this._native = hdb.createClient(creds)
this._native.setAutoCommit(false)
this._native.on('close', () => this.destroy?.())

this.connected = false
}
Expand All @@ -31,6 +32,10 @@ class HDBDriver extends driver {
Object.keys(variables).forEach(k => clientInfo.setProperty(k, variables[k]))
}

async validate() {
return this._native.readyState === 'connected'
}

/**
* Connects the driver using the provided credentials
* @returns {Promise<any>}
Expand Down