From aee7274fdb86c41471526e45dcec55a5d21276f2 Mon Sep 17 00:00:00 2001 From: lutovich Date: Thu, 27 Apr 2017 14:51:23 +0200 Subject: [PATCH 1/2] Read in absence of viable writer Previously driver did not allow reads and writes when received routing table did not contains both routers, readers and writers. This was inconsistent with Causal Cluster which allows reads when leader is absent. Leader might be unavailable for a long time (when there is a DC failure, etc.) so it makes sense to allow clients to perform read activity. This commit makes driver accept routing table with no writers and allow clients to perform read operations when writers are not available. It might be problematic when there is a cluster partition and one partition contains majority. For this case special care must be taken so driver does not get stuck talking only to the smaller partition which only knows about itself. This is done on best effort basis - driver tries to contact seed router if previously accepted routing table did not contain writes. --- src/v1/internal/connection-providers.js | 109 +++++--- src/v1/internal/round-robin-array.js | 4 + src/v1/internal/routing-table.js | 21 +- test/internal/connection-providers.test.js | 239 ++++++++++-------- test/internal/round-robin-array.test.js | 7 +- test/internal/routing-table.test.js | 31 ++- .../boltkit/dead_routing_server.script | 7 + .../boltkit/discover_no_writers.script | 9 + test/v1/routing.driver.boltkit.it.js | 171 ++++++++++++- 9 files changed, 440 insertions(+), 158 deletions(-) create mode 100644 test/resources/boltkit/dead_routing_server.script create mode 100644 test/resources/boltkit/discover_no_writers.script diff --git a/src/v1/internal/connection-providers.js b/src/v1/internal/connection-providers.js index c3aff54bc..b9b633011 100644 --- a/src/v1/internal/connection-providers.js +++ b/src/v1/internal/connection-providers.js @@ -17,7 +17,7 @@ * limitations under the License. */ -import {newError, SERVICE_UNAVAILABLE} from '../error'; +import {newError, SERVICE_UNAVAILABLE, SESSION_EXPIRED} from '../error'; import {READ, WRITE} from '../driver'; import Session from '../session'; import RoundRobinArray from './round-robin-array'; @@ -70,16 +70,17 @@ export class LoadBalancer extends ConnectionProvider { this._connectionPool = connectionPool; this._driverOnErrorCallback = driverOnErrorCallback; this._hostNameResolver = LoadBalancer._createHostNameResolver(); + this._useSeedRouter = false; } - acquireConnection(mode) { - const connectionPromise = this._freshRoutingTable().then(routingTable => { - if (mode === READ) { + acquireConnection(accessMode) { + const connectionPromise = this._freshRoutingTable(accessMode).then(routingTable => { + if (accessMode === READ) { return this._acquireConnectionToServer(routingTable.readers, 'read'); - } else if (mode === WRITE) { + } else if (accessMode === WRITE) { return this._acquireConnectionToServer(routingTable.writers, 'write'); } else { - throw newError('Illegal mode ' + mode); + throw newError('Illegal mode ' + accessMode); } }); return this._withAdditionalOnErrorCallback(connectionPromise, this._driverOnErrorCallback); @@ -97,15 +98,17 @@ export class LoadBalancer extends ConnectionProvider { _acquireConnectionToServer(serversRoundRobinArray, serverName) { const address = serversRoundRobinArray.next(); if (!address) { - return Promise.reject(newError('No ' + serverName + ' servers available', SERVICE_UNAVAILABLE)); + return Promise.reject(newError( + `Failed to obtain connection towards ${serverName} server. Known routing table is: ${this._routingTable}`, + SESSION_EXPIRED)); } return this._connectionPool.acquire(address); } - _freshRoutingTable() { + _freshRoutingTable(accessMode) { const currentRoutingTable = this._routingTable; - if (!currentRoutingTable.isStale()) { + if (!currentRoutingTable.isStaleFor(accessMode)) { return Promise.resolve(currentRoutingTable); } return this._refreshRoutingTable(currentRoutingTable); @@ -114,48 +117,73 @@ export class LoadBalancer extends ConnectionProvider { _refreshRoutingTable(currentRoutingTable) { const knownRouters = currentRoutingTable.routers.toArray(); - return this._fetchNewRoutingTable(knownRouters, currentRoutingTable).then(newRoutingTable => { - if (LoadBalancer._isValidRoutingTable(newRoutingTable)) { - // one of the known routers returned a valid routing table - use it + if (this._useSeedRouter) { + return this._fetchRoutingTableFromSeedRouterFallbackToKnownRouters(knownRouters, currentRoutingTable); + } + return this._fetchRoutingTableFromKnownRoutersFallbackToSeedRouter(knownRouters, currentRoutingTable); + } + + _fetchRoutingTableFromSeedRouterFallbackToKnownRouters(knownRouters, currentRoutingTable) { + // we start with seed router, no routers were probed before + const seenRouters = []; + return this._fetchRoutingTableUsingSeedRouter(seenRouters, this._seedRouter).then(newRoutingTable => { + if (newRoutingTable) { + this._useSeedRouter = false; return newRoutingTable; } - if (!newRoutingTable) { - // returned routing table was undefined, this means a connection error happened and the last known - // router did not return a valid routing table, so we need to forget it - const lastRouterIndex = knownRouters.length - 1; - LoadBalancer._forgetRouter(currentRoutingTable, knownRouters, lastRouterIndex); + // seed router did not return a valid routing table - try to use other known routers + return this._fetchRoutingTableUsingKnownRouters(knownRouters, currentRoutingTable); + }).then(newRoutingTable => { + this._applyRoutingTableIfPossible(newRoutingTable); + return newRoutingTable; + }); + } + + _fetchRoutingTableFromKnownRoutersFallbackToSeedRouter(knownRouters, currentRoutingTable) { + return this._fetchRoutingTableUsingKnownRouters(knownRouters, currentRoutingTable).then(newRoutingTable => { + if (newRoutingTable) { + return newRoutingTable; } // none of the known routers returned a valid routing table - try to use seed router address for rediscovery - return this._fetchNewRoutingTableUsingSeedRouterAddress(knownRouters, this._seedRouter); + return this._fetchRoutingTableUsingSeedRouter(knownRouters, this._seedRouter); }).then(newRoutingTable => { - if (LoadBalancer._isValidRoutingTable(newRoutingTable)) { - this._updateRoutingTable(newRoutingTable); + this._applyRoutingTableIfPossible(newRoutingTable); + return newRoutingTable; + }); + } + + _fetchRoutingTableUsingKnownRouters(knownRouters, currentRoutingTable) { + return this._fetchRoutingTable(knownRouters, currentRoutingTable).then(newRoutingTable => { + if (newRoutingTable) { + // one of the known routers returned a valid routing table - use it return newRoutingTable; } - // none of the existing routers returned valid routing table, throw exception - throw newError('Could not perform discovery. No routing servers available.', SERVICE_UNAVAILABLE); + // returned routing table was undefined, this means a connection error happened and the last known + // router did not return a valid routing table, so we need to forget it + const lastRouterIndex = knownRouters.length - 1; + LoadBalancer._forgetRouter(currentRoutingTable, knownRouters, lastRouterIndex); + + return null; }); } - _fetchNewRoutingTableUsingSeedRouterAddress(knownRouters, seedRouter) { + _fetchRoutingTableUsingSeedRouter(seenRouters, seedRouter) { return this._hostNameResolver.resolve(seedRouter).then(resolvedRouterAddresses => { // filter out all addresses that we've already tried - const newAddresses = resolvedRouterAddresses.filter(address => knownRouters.indexOf(address) < 0); - return this._fetchNewRoutingTable(newAddresses, null); + const newAddresses = resolvedRouterAddresses.filter(address => seenRouters.indexOf(address) < 0); + return this._fetchRoutingTable(newAddresses, null); }); } - _fetchNewRoutingTable(routerAddresses, routingTable) { + _fetchRoutingTable(routerAddresses, routingTable) { return routerAddresses.reduce((refreshedTablePromise, currentRouter, currentIndex) => { return refreshedTablePromise.then(newRoutingTable => { if (newRoutingTable) { - if (!newRoutingTable.writers.isEmpty()) { - // valid routing table was fetched - just return it, try next router otherwise - return newRoutingTable; - } + // valid routing table was fetched - just return it, try next router otherwise + return newRoutingTable; } else { // returned routing table was undefined, this means a connection error happened and we need to forget the // previous router and try the next one @@ -179,6 +207,23 @@ export class LoadBalancer extends ConnectionProvider { return new Session(READ, connectionProvider); } + _applyRoutingTableIfPossible(newRoutingTable) { + if (!newRoutingTable) { + // none of routing servers returned valid routing table, throw exception + throw newError( + `Could not perform discovery. No routing servers available. Known routing table: ${this._routingTable}`, + SERVICE_UNAVAILABLE); + } + + if (newRoutingTable.writers.isEmpty()) { + // use seed router next time. this is important when cluster is partitioned. it tries to make sure driver + // does not always get routing table without writers because it talks exclusively to a minority partition + this._useSeedRouter = true; + } + + this._updateRoutingTable(newRoutingTable); + } + _updateRoutingTable(newRoutingTable) { const currentRoutingTable = this._routingTable; @@ -190,10 +235,6 @@ export class LoadBalancer extends ConnectionProvider { this._routingTable = newRoutingTable; } - static _isValidRoutingTable(routingTable) { - return routingTable && !routingTable.writers.isEmpty(); - } - static _forgetRouter(routingTable, routersArray, routerIndex) { const address = routersArray[routerIndex]; if (routingTable && address) { diff --git a/src/v1/internal/round-robin-array.js b/src/v1/internal/round-robin-array.js index d9ef40432..fa8e0ca87 100644 --- a/src/v1/internal/round-robin-array.js +++ b/src/v1/internal/round-robin-array.js @@ -59,4 +59,8 @@ export default class RoundRobinArray { remove(item) { this._items = this._items.filter(element => element !== item); } + + toString() { + return JSON.stringify(this._items); + } } diff --git a/src/v1/internal/routing-table.js b/src/v1/internal/routing-table.js index b1cb0a888..34d180006 100644 --- a/src/v1/internal/routing-table.js +++ b/src/v1/internal/routing-table.js @@ -18,6 +18,7 @@ */ import {int} from '../integer'; import RoundRobinArray from './round-robin-array'; +import {READ, WRITE} from '../driver'; const MIN_ROUTERS = 1; @@ -53,14 +54,28 @@ export default class RoutingTable { return Array.from(oldServers); } - isStale() { + /** + * Check if this routing table is fresh to perform the required operation. + * @param {string} accessMode the type of operation. Allowed values are {@link READ} and {@link WRITE}. + * @return {boolean} true when this table contains servers to serve the required operation, + * false otherwise. + */ + isStaleFor(accessMode) { return this.expirationTime.lessThan(Date.now()) || this.routers.size() < MIN_ROUTERS || - this.readers.isEmpty() || - this.writers.isEmpty(); + accessMode === READ && this.readers.isEmpty() || + accessMode === WRITE && this.writers.isEmpty(); } _allServers() { return [...this.routers.toArray(), ...this.readers.toArray(), ...this.writers.toArray()]; } + + toString() { + return `RoutingTable[` + + `expirationTime=${this.expirationTime}, ` + + `routers=${this.routers}, ` + + `readers=${this.readers}, ` + + `writers=${this.writers}]`; + } } diff --git a/test/internal/connection-providers.test.js b/test/internal/connection-providers.test.js index c7b35d33b..14e291f17 100644 --- a/test/internal/connection-providers.test.js +++ b/test/internal/connection-providers.test.js @@ -19,7 +19,7 @@ import {READ, WRITE} from '../../src/v1/driver'; import Integer, {int} from '../../src/v1/integer'; -import {SERVICE_UNAVAILABLE} from '../../src/v1/error'; +import {SERVICE_UNAVAILABLE, SESSION_EXPIRED} from '../../src/v1/error'; import RoutingTable from '../../src/v1/internal/routing-table'; import RoundRobinArray from '../../src/v1/internal/round-robin-array'; import {DirectConnectionProvider, LoadBalancer} from '../../src/v1/internal/connection-providers'; @@ -291,7 +291,7 @@ describe('LoadBalancer', () => { }); }); - it('refreshes stale routing table to get write connection', done => { + it('refreshes stale routing table to get write connection when one router fails', done => { const pool = newPool(); const updatedRoutingTable = newRoutingTable( ['server-A', 'server-B'], @@ -323,27 +323,22 @@ describe('LoadBalancer', () => { }); }); - it('refreshes stale routing table to get read connection when one router returns illegal response', done => { + it('refreshes routing table without readers to get read connection', done => { const pool = newPool(); - const newIllegalRoutingTable = newRoutingTable( - ['server-A', 'server-B'], - ['server-C', 'server-D'], - [] // no writers - table is illegal and should be skipped - ); - const newLegalRoutingTable = newRoutingTable( + const updatedRoutingTable = newRoutingTable( ['server-A', 'server-B'], ['server-C', 'server-D'], ['server-E', 'server-F'] ); const loadBalancer = newLoadBalancer( ['server-1', 'server-2'], + [], // no readers ['server-3', 'server-4'], - ['server-5', 'server-6'], pool, - int(0), // expired routing table + Integer.MAX_VALUE, { - 'server-1': newIllegalRoutingTable, - 'server-2': newLegalRoutingTable, + 'server-1': null, // returns no routing table + 'server-2': updatedRoutingTable, } ); @@ -360,14 +355,9 @@ describe('LoadBalancer', () => { }); }); - it('refreshes stale routing table to get write connection when one router returns illegal response', done => { + it('refreshes routing table without writers to get write connection', done => { const pool = newPool(); - const newIllegalRoutingTable = newRoutingTable( - ['server-A', 'server-B'], - ['server-C', 'server-D'], - [] // no writers - table is illegal and should be skipped - ); - const newLegalRoutingTable = newRoutingTable( + const updatedRoutingTable = newRoutingTable( ['server-A', 'server-B'], ['server-C', 'server-D'], ['server-E', 'server-F'] @@ -375,12 +365,12 @@ describe('LoadBalancer', () => { const loadBalancer = newLoadBalancer( ['server-1', 'server-2'], ['server-3', 'server-4'], - ['server-5', 'server-6'], + [], // no writers pool, int(0), // expired routing table { - 'server-1': newIllegalRoutingTable, - 'server-2': newLegalRoutingTable, + 'server-1': null, // returns no routing table + 'server-2': updatedRoutingTable, } ); @@ -435,11 +425,11 @@ describe('LoadBalancer', () => { }); }); - it('throws when all routers return illegal routing tables while getting read connection', done => { - const newIllegalRoutingTable = newRoutingTable( + it('throws when all routers return routing tables without readers while getting read connection', done => { + const updatedRoutingTable = newRoutingTable( ['server-A', 'server-B'], - ['server-C', 'server-D'], - [] // no writers - table is illegal and should be skipped + [], // no readers - table can't satisfy connection requirement + ['server-C', 'server-D'] ); const loadBalancer = newLoadBalancer( ['server-1', 'server-2'], @@ -448,22 +438,22 @@ describe('LoadBalancer', () => { newPool(), int(0), // expired routing table { - 'server-1': newIllegalRoutingTable, - 'server-2': newIllegalRoutingTable + 'server-1': updatedRoutingTable, + 'server-2': updatedRoutingTable } ); loadBalancer.acquireConnection(READ).catch(error => { - expect(error.code).toEqual(SERVICE_UNAVAILABLE); + expect(error.code).toEqual(SESSION_EXPIRED); done(); }); }); - it('throws when all routers return illegal routing tables while getting write connection', done => { - const newIllegalRoutingTable = newRoutingTable( + it('throws when all routers return routing tables without writers while getting write connection', done => { + const updatedRoutingTable = newRoutingTable( ['server-A', 'server-B'], ['server-C', 'server-D'], - [] // no writers - table is illegal and should be skipped + [] // no writers - table can't satisfy connection requirement ); const loadBalancer = newLoadBalancer( ['server-1', 'server-2'], @@ -472,13 +462,13 @@ describe('LoadBalancer', () => { newPool(), int(0), // expired routing table { - 'server-1': newIllegalRoutingTable, - 'server-2': newIllegalRoutingTable + 'server-1': updatedRoutingTable, + 'server-2': updatedRoutingTable } ); loadBalancer.acquireConnection(WRITE).catch(error => { - expect(error.code).toEqual(SERVICE_UNAVAILABLE); + expect(error.code).toEqual(SESSION_EXPIRED); done(); }); }); @@ -582,12 +572,7 @@ describe('LoadBalancer', () => { }); }); - it('uses seed router address when all existing routers failed', done => { - const illegalRoutingTable = newRoutingTable( - ['server-A', 'server-B'], - ['server-C', 'server-D'], - [] // no writers - table is illegal and should be skipped - ); + it('uses seed router address when all existing routers fail', done => { const updatedRoutingTable = newRoutingTable( ['server-A', 'server-B', 'server-C'], ['server-D', 'server-E'], @@ -602,7 +587,7 @@ describe('LoadBalancer', () => { int(0), // expired routing table { 'server-1': null, // returns no routing table - 'server-2': illegalRoutingTable, + 'server-2': null, // returns no routing table 'server-3': null, // returns no routing table 'server-0': updatedRoutingTable } @@ -624,12 +609,7 @@ describe('LoadBalancer', () => { }); }); - it('uses resolved seed router address when all existing routers failed', done => { - const illegalRoutingTable = newRoutingTable( - ['server-A'], - ['server-B'], - [] // no writers - table is illegal and should be skipped - ); + it('uses resolved seed router address when all existing routers fail', done => { const updatedRoutingTable = newRoutingTable( ['server-A', 'server-B'], ['server-C', 'server-D'], @@ -643,8 +623,8 @@ describe('LoadBalancer', () => { ['server-6', 'server-7'], int(0), // expired routing table { - 'server-1': illegalRoutingTable, - 'server-2': illegalRoutingTable, + 'server-1': null, // returns no routing table + 'server-2': null, // returns no routing table 'server-3': null, // returns no routing table 'server-01': updatedRoutingTable } @@ -666,12 +646,7 @@ describe('LoadBalancer', () => { }); }); - it('uses resolved seed router address that returns correct routing table when all existing routers failed', done => { - const illegalRoutingTable = newRoutingTable( - ['server-A', 'server-B'], - ['server-C', 'server-D'], - [] // no writers - table is illegal and should be skipped - ); + it('uses resolved seed router address that returns correct routing table when all existing routers fail', done => { const updatedRoutingTable = newRoutingTable( ['server-A', 'server-B'], ['server-C'], @@ -685,9 +660,9 @@ describe('LoadBalancer', () => { ['server-3'], int(0), // expired routing table { - 'server-1': illegalRoutingTable, + 'server-1': null, // returns no routing table 'server-01': null, // returns no routing table - 'server-02': illegalRoutingTable, + 'server-02': null, // returns no routing table 'server-03': updatedRoutingTable } ); @@ -708,9 +683,7 @@ describe('LoadBalancer', () => { }); }); - it('fails when existing routers fail and seed router returns an invalid routing table', done => { - const emptyIllegalRoutingTable = newRoutingTable([], [], []); - + it('fails when both existing routers and seed router fail to return a routing table', done => { const loadBalancer = newLoadBalancerWithSeedRouter( 'server-0', ['server-0'], // seed router address resolves just to itself ['server-1', 'server-2', 'server-3'], @@ -718,10 +691,10 @@ describe('LoadBalancer', () => { ['server-6'], int(0), // expired routing table { - 'server-1': emptyIllegalRoutingTable, + 'server-1': null, // returns no routing table 'server-2': null, // returns no routing table 'server-3': null, // returns no routing table - 'server-0': emptyIllegalRoutingTable + 'server-0': null // returns no routing table } ); @@ -729,7 +702,7 @@ describe('LoadBalancer', () => { expect(error.code).toEqual(SERVICE_UNAVAILABLE); expectRoutingTable(loadBalancer, - ['server-1'], // only server-1 is in the table, it returned a routing table which turned out to be invalid + [], // all routers were forgotten because they failed ['server-4', 'server-5'], ['server-6'], ); @@ -738,7 +711,7 @@ describe('LoadBalancer', () => { expect(error.code).toEqual(SERVICE_UNAVAILABLE); expectRoutingTable(loadBalancer, - ['server-1'], // only server-1 is in the table, it returned a routing table which turned out to be invalid + [], // all routers were forgotten because they failed ['server-4', 'server-5'], ['server-6'], ); @@ -748,13 +721,7 @@ describe('LoadBalancer', () => { }); }); - it('fails when existing routers fail and resolved seed router returns an invalid routing table', done => { - const illegalRoutingTable = newRoutingTable( - ['server-A', 'server-B'], - ['server-C', 'server-D'], - [] // no writers - table is illegal and should be skipped - ); - + it('fails when both existing routers and resolved seed router fail to return a routing table', done => { const loadBalancer = newLoadBalancerWithSeedRouter( 'server-0', ['server-01'], // seed router address resolves to a different one ['server-1', 'server-2'], @@ -763,7 +730,7 @@ describe('LoadBalancer', () => { int(0), // expired routing table { 'server-1': null, // returns no routing table - 'server-2': illegalRoutingTable, + 'server-2': null, // returns no routing table 'server-01': null // returns no routing table } ); @@ -772,7 +739,7 @@ describe('LoadBalancer', () => { expect(error.code).toEqual(SERVICE_UNAVAILABLE); expectRoutingTable(loadBalancer, - ['server-2'], // only server-2 is in the table, it returned a routing table which turned out to be invalid + [], // all routers were forgotten because they failed ['server-3'], ['server-4'], ); @@ -781,7 +748,7 @@ describe('LoadBalancer', () => { expect(error.code).toEqual(SERVICE_UNAVAILABLE); expectRoutingTable(loadBalancer, - ['server-2'], // only server-2 is in the table, it returned a routing table which turned out to be invalid + [], // all routers were forgotten because they failed ['server-3'], ['server-4'], ); @@ -791,13 +758,7 @@ describe('LoadBalancer', () => { }); }); - it('fails when existing routers fail and all resolved seed routers return an invalid routing table', done => { - const illegalRoutingTable = newRoutingTable( - ['server-A', 'server-B'], - ['server-C', 'server-D'], - [] // no writers - table is illegal and should be skipped - ); - + it('fails when both existing routers and all resolved seed routers fail to return a routing table', done => { const loadBalancer = newLoadBalancerWithSeedRouter( 'server-0', ['server-02', 'server-01'], // seed router address resolves to 2 different addresses ['server-1', 'server-2', 'server-3'], @@ -808,7 +769,7 @@ describe('LoadBalancer', () => { 'server-1': null, // returns no routing table 'server-2': null, // returns no routing table 'server-3': null, // returns no routing table - 'server-01': illegalRoutingTable, + 'server-01': null, // returns no routing table 'server-02': null // returns no routing table } ); @@ -904,12 +865,7 @@ describe('LoadBalancer', () => { }); }); - it('uses resolved seed router that returns correct routing table when no existing routers exist', done => { - const illegalRoutingTable = newRoutingTable( - ['server-A'], - ['server-B'], - [] // no writers - table is illegal and should be skipped - ); + it('uses resolved seed router that returns routing table when no existing routers exist', done => { const updatedRoutingTable = newRoutingTable( ['server-A', 'server-B', 'server-C'], ['server-D', 'server-E'], @@ -923,8 +879,8 @@ describe('LoadBalancer', () => { ['server-2', 'server-3'], Integer.MAX_VALUE, // not expired { - 'server-01': null, - 'server-02': illegalRoutingTable, + 'server-01': null, // returns no routing table + 'server-02': null, // returns no routing table 'server-03': updatedRoutingTable } ); @@ -946,11 +902,6 @@ describe('LoadBalancer', () => { }); it('ignores already probed routers after seed router resolution', done => { - const illegalRoutingTable = newRoutingTable( - ['server-A'], - ['server-B'], - [] // no writers - table is illegal and should be skipped - ); const updatedRoutingTable = newRoutingTable( ['server-A', 'server-B'], ['server-C', 'server-D'], @@ -964,9 +915,9 @@ describe('LoadBalancer', () => { ['server-5', 'server-6'], int(0), // expired routing table { - 'server-1': null, - 'server-01': null, - 'server-2': illegalRoutingTable, + 'server-1': null, // returns no routing table + 'server-01': null, // returns no routing table + 'server-2': null, // returns no routing table 'server-02': updatedRoutingTable } ); @@ -996,11 +947,11 @@ describe('LoadBalancer', () => { }); }); - it('throws service unavailable when refreshed routing table has no readers', done => { + it('throws session expired when refreshed routing table has no readers', done => { const pool = newPool(); const updatedRoutingTable = newRoutingTable( ['server-A', 'server-B'], - [], + [], // no readers ['server-C', 'server-D'] ); const loadBalancer = newLoadBalancer( @@ -1015,11 +966,89 @@ describe('LoadBalancer', () => { ); loadBalancer.acquireConnection(READ).catch(error => { - expect(error.code).toEqual(SERVICE_UNAVAILABLE); + expect(error.code).toEqual(SESSION_EXPIRED); done(); }); }); + it('throws session expired when refreshed routing table has no writers', done => { + const pool = newPool(); + const updatedRoutingTable = newRoutingTable( + ['server-A', 'server-B'], + ['server-C', 'server-D'], + [] // no writers + ); + const loadBalancer = newLoadBalancer( + ['server-1', 'server-2'], + ['server-3', 'server-4'], + ['server-5', 'server-6'], + pool, + int(0), // expired routing table + { + 'server-1': updatedRoutingTable, + } + ); + + loadBalancer.acquireConnection(WRITE).catch(error => { + expect(error.code).toEqual(SESSION_EXPIRED); + done(); + }); + }); + + it('should use resolved seed router after accepting table with no writers', done => { + const routingTable1 = newRoutingTable( + ['server-A', 'server-B'], + ['server-C', 'server-D'], + [] // no writers + ); + const routingTable2 = newRoutingTable( + ['server-AA', 'server-BB'], + ['server-CC', 'server-DD'], + ['server-EE'] + ); + + const loadBalancer = newLoadBalancerWithSeedRouter( + 'server-0', ['server-02', 'server-01'], // seed router address resolves to 2 different addresses + ['server-1'], + ['server-2', 'server-3'], + ['server-4', 'server-5'], + int(0), // expired routing table + { + 'server-1': routingTable1, + 'server-A': routingTable1, + 'server-B': routingTable1, + 'server-01': null, // returns no routing table + 'server-02': routingTable2 + } + ); + + loadBalancer.acquireConnection(READ).then(connection1 => { + expect(connection1.address).toEqual('server-C'); + + loadBalancer.acquireConnection(READ).then(connection2 => { + expect(connection2.address).toEqual('server-D'); + + expectRoutingTable(loadBalancer, + ['server-A', 'server-B'], + ['server-C', 'server-D'], + [] + ); + + loadBalancer.acquireConnection(WRITE).then(connection3 => { + expect(connection3.address).toEqual('server-EE'); + + expectRoutingTable(loadBalancer, + ['server-AA', 'server-BB'], + ['server-CC', 'server-DD'], + ['server-EE'] + ); + + done(); + }); + }); + }); + }); + }); function newDirectConnectionProvider(address, pool) { @@ -1062,12 +1091,12 @@ function newRoutingTable(routers, readers, writers, expirationTime = Integer.MAX } function setupLoadBalancerToRememberRouters(loadBalancer, routersArray) { - const originalFetch = loadBalancer._fetchNewRoutingTable.bind(loadBalancer); + const originalFetch = loadBalancer._fetchRoutingTable.bind(loadBalancer); const rememberingFetch = (routerAddresses, routingTable) => { routersArray.push(routerAddresses); return originalFetch(routerAddresses, routingTable); }; - loadBalancer._fetchNewRoutingTable = rememberingFetch; + loadBalancer._fetchRoutingTable = rememberingFetch; } function newPool() { diff --git a/test/internal/round-robin-array.test.js b/test/internal/round-robin-array.test.js index 1064b47d2..028bdc090 100644 --- a/test/internal/round-robin-array.test.js +++ b/test/internal/round-robin-array.test.js @@ -16,7 +16,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import RoundRobinArray from "../../lib/v1/internal/round-robin-array"; +import RoundRobinArray from '../../lib/v1/internal/round-robin-array'; describe('round-robin-array', () => { @@ -198,4 +198,9 @@ describe('round-robin-array', () => { expect(array.next()).toEqual(3); }); + it('should have correct toString ', () => { + const array = new RoundRobinArray([1, 2, 3]); + expect(array.toString()).toEqual('[1,2,3]'); + }); + }); diff --git a/test/internal/routing-table.test.js b/test/internal/routing-table.test.js index 8ea5f343f..fb64fd33c 100644 --- a/test/internal/routing-table.test.js +++ b/test/internal/routing-table.test.js @@ -19,42 +19,50 @@ import RoutingTable from '../../src/v1/internal/routing-table'; import RoundRobinArray from '../../src/v1/internal/round-robin-array'; import {int} from '../../src/v1/integer'; +import {READ, WRITE} from '../../src/v1/driver'; describe('routing-table', () => { it('should not be stale when has routers, readers, writers and future expiration date', () => { const table = createTable([1, 2], [3, 4], [5, 6], notExpired()); - expect(table.isStale()).toBeFalsy(); + expect(table.isStaleFor(READ)).toBeFalsy(); + expect(table.isStaleFor(WRITE)).toBeFalsy(); }); it('should be stale when expiration date in the past', () => { const table = createTable([1, 2], [1, 2], [1, 2], expired()); - expect(table.isStale()).toBeTruthy(); + expect(table.isStaleFor(READ)).toBeTruthy(); + expect(table.isStaleFor(WRITE)).toBeTruthy(); }); it('should not be stale when has single router', () => { const table = createTable([1], [2, 3], [4, 5], notExpired()); - expect(table.isStale()).toBeFalsy(); + expect(table.isStaleFor(READ)).toBeFalsy(); + expect(table.isStaleFor(WRITE)).toBeFalsy(); }); - it('should be stale when no readers', () => { + it('should be stale for reads but not writes when no readers', () => { const table = createTable([1, 2], [], [3, 4], notExpired()); - expect(table.isStale()).toBeTruthy(); + expect(table.isStaleFor(READ)).toBeTruthy(); + expect(table.isStaleFor(WRITE)).toBeFalsy(); }); - it('should be stale when no writers', () => { + it('should be stale for writes but not reads when no writers', () => { const table = createTable([1, 2], [3, 4], [], notExpired()); - expect(table.isStale()).toBeTruthy(); + expect(table.isStaleFor(READ)).toBeFalsy(); + expect(table.isStaleFor(WRITE)).toBeTruthy(); }); it('should not be stale with single reader', () => { const table = createTable([1, 2], [3], [4, 5], notExpired()); - expect(table.isStale()).toBeFalsy(); + expect(table.isStaleFor(READ)).toBeFalsy(); + expect(table.isStaleFor(WRITE)).toBeFalsy(); }); it('should not be stale with single writer', () => { const table = createTable([1, 2], [3, 4], [5], notExpired()); - expect(table.isStale()).toBeFalsy(); + expect(table.isStaleFor(READ)).toBeFalsy(); + expect(table.isStaleFor(WRITE)).toBeFalsy(); }); it('should forget reader, writer but not router', () => { @@ -161,6 +169,11 @@ describe('routing-table', () => { expect(servers).toEqual([11, 22, 33, 44]); }); + it('should have correct toString', () => { + const table = createTable([1, 2], [3, 4], [5, 6], 42); + expect(table.toString()).toEqual('RoutingTable[expirationTime=42, routers=[1,2], readers=[3,4], writers=[5,6]]'); + }); + function expired() { return Date.now() - 3600; // expired an hour ago } diff --git a/test/resources/boltkit/dead_routing_server.script b/test/resources/boltkit/dead_routing_server.script new file mode 100644 index 000000000..2791eaea9 --- /dev/null +++ b/test/resources/boltkit/dead_routing_server.script @@ -0,0 +1,7 @@ +!: AUTO INIT +!: AUTO RESET +!: AUTO PULL_ALL + +C: RUN "CALL dbms.cluster.routing.getServers" {} +C: PULL_ALL +S: diff --git a/test/resources/boltkit/discover_no_writers.script b/test/resources/boltkit/discover_no_writers.script new file mode 100644 index 000000000..20f69dde4 --- /dev/null +++ b/test/resources/boltkit/discover_no_writers.script @@ -0,0 +1,9 @@ +!: AUTO INIT +!: AUTO RESET +!: AUTO PULL_ALL + +C: RUN "CALL dbms.cluster.routing.getServers" {} + PULL_ALL +S: SUCCESS {"fields": ["ttl", "servers"]} + RECORD [9223372036854775807, [{"addresses": [],"role": "WRITE"}, {"addresses": ["127.0.0.1:9002","127.0.0.1:9003"], "role": "READ"},{"addresses": ["127.0.0.1:9004","127.0.0.1:9005"], "role": "ROUTE"}]] + SUCCESS {} diff --git a/test/v1/routing.driver.boltkit.it.js b/test/v1/routing.driver.boltkit.it.js index c5f700284..39d0ed4c0 100644 --- a/test/v1/routing.driver.boltkit.it.js +++ b/test/v1/routing.driver.boltkit.it.js @@ -670,7 +670,7 @@ describe('routing driver', () => { // When const session = driver.session(neo4j.session.WRITE); session.run("MATCH (n) RETURN n.name").catch(err => { - expect(err.code).toEqual(neo4j.error.SERVICE_UNAVAILABLE); + expect(err.code).toEqual(neo4j.error.SESSION_EXPIRED); driver.close(); seedServer.exit(code => { expect(code).toEqual(0); @@ -680,7 +680,7 @@ describe('routing driver', () => { }); }); - it('should try next router when no writers', done => { + it('should try next router when current router fails to return a routing table', done => { if (!boltkit.BoltKitSupport) { done(); return; @@ -688,9 +688,9 @@ describe('routing driver', () => { const kit = new boltkit.BoltKit(); const server1 = kit.start('./test/resources/boltkit/routing_table_with_zero_ttl.script', 9999); - const server2 = kit.start('./test/resources/boltkit/no_writers.script', 9091); - const server3 = kit.start('./test/resources/boltkit/no_writers.script', 9092); - const server4 = kit.start('./test/resources/boltkit/no_writers.script', 9093); + const server2 = kit.start('./test/resources/boltkit/dead_routing_server.script', 9091); + const server3 = kit.start('./test/resources/boltkit/dead_routing_server.script', 9092); + const server4 = kit.start('./test/resources/boltkit/dead_routing_server.script', 9093); kit.run(() => { const driver = newDriver('bolt+routing://127.0.0.1:9999'); @@ -708,7 +708,8 @@ describe('routing driver', () => { expect(result2.summary.server.address).toEqual('127.0.0.1:9999'); session2.close(); - memorizingRoutingTable.assertForgotRouters([]); + // returned routers failed to respond and should have been forgotten + memorizingRoutingTable.assertForgotRouters(['127.0.0.1:9091', '127.0.0.1:9092', '127.0.0.1:9093']); assertHasRouters(driver, ['127.0.0.1:9999']); driver.close(); @@ -1672,6 +1673,164 @@ describe('routing driver', () => { }); }); + it('should use routing table without writers for reads', done => { + const kit = new boltkit.BoltKit(); + const router = kit.start('./test/resources/boltkit/discover_no_writers.script', 9010); + const reader = kit.start('./test/resources/boltkit/read_server.script', 9002); + + kit.run(() => { + const driver = newDriver('bolt+routing://127.0.0.1:9010'); + + const session = driver.session(READ); + session.run('MATCH (n) RETURN n.name').then(result => { + session.close(() => { + expect(result.records.map(record => record.get(0))).toEqual(['Bob', 'Alice', 'Tina']); + + driver.close(); + + router.exit(code1 => { + reader.exit(code2 => { + expect(code1).toEqual(0); + expect(code2).toEqual(0); + done(); + }); + }); + }); + }); + }); + }); + + it('should serve reads but fail writes when no writers available', done => { + const kit = new boltkit.BoltKit(); + const router1 = kit.start('./test/resources/boltkit/discover_no_writers.script', 9010); + const router2 = kit.start('./test/resources/boltkit/discover_no_writers.script', 9004); + const reader = kit.start('./test/resources/boltkit/read_server.script', 9003); + + kit.run(() => { + const driver = newDriver('bolt+routing://127.0.0.1:9010'); + + const readSession = driver.session(); + + readSession.readTransaction(tx => tx.run('MATCH (n) RETURN n.name')).then(result => { + readSession.close(() => { + expect(result.records.map(record => record.get(0))).toEqual(['Bob', 'Alice', 'Tina']); + + const writeSession = driver.session(WRITE); + writeSession.run('CREATE (n {name:\'Bob\'})').catch(error => { + expect(error.code).toEqual(neo4j.error.SESSION_EXPIRED); + + driver.close(); + + router1.exit(code1 => { + router2.exit(code2 => { + reader.exit(code3 => { + expect(code1).toEqual(0); + expect(code2).toEqual(0); + expect(code3).toEqual(0); + done(); + }); + }); + }); + }); + }); + }); + }); + }); + + it('should accept routing table without writers and then rediscover', done => { + const kit = new boltkit.BoltKit(); + + // first router does not have itself in the resulting routing table so connection + // towards it will be closed after rediscovery + const router1 = kit.start('./test/resources/boltkit/discover_no_writers.script', 9010); + let router2 = null; + const reader = kit.start('./test/resources/boltkit/read_server.script', 9003); + const writer = kit.start('./test/resources/boltkit/write_server.script', 9007); + + kit.run(() => { + const driver = newDriver('bolt+routing://127.0.0.1:9010'); + + const readSession = driver.session(); + + readSession.readTransaction(tx => tx.run('MATCH (n) RETURN n.name')).then(result => { + readSession.close(() => { + expect(result.records.map(record => record.get(0))).toEqual(['Bob', 'Alice', 'Tina']); + + // start another router which knows about writes, use same address as the initial router + router2 = kit.start('./test/resources/boltkit/acquire_endpoints.script', 9010); + kit.run(() => { + const writeSession = driver.session(WRITE); + writeSession.run('CREATE (n {name:\'Bob\'})').then(result => { + writeSession.close(() => { + expect(result.records).toEqual([]); + + driver.close(); + + router1.exit(code1 => { + router2.exit(code2 => { + reader.exit(code3 => { + writer.exit(code4 => { + expect(code1).toEqual(0); + expect(code2).toEqual(0); + expect(code3).toEqual(0); + expect(code4).toEqual(0); + done(); + }); + }); + }); + }); + }); + }); + }); + }); + }); + }); + }); + + it('should use resolved seed router for discovery after accepting a table without writers', done => { + const kit = new boltkit.BoltKit(); + const seedRouter = kit.start('./test/resources/boltkit/no_writers.script', 9010); + const resolvedSeedRouter = kit.start('./test/resources/boltkit/acquire_endpoints.script', 9020); + const reader = kit.start('./test/resources/boltkit/read_server.script', 9005); + const writer = kit.start('./test/resources/boltkit/write_server.script', 9007); + + kit.run(() => { + const driver = newDriver('bolt+routing://127.0.0.1:9010'); + + const readSession = driver.session(READ); + readSession.run('MATCH (n) RETURN n.name').then(result => { + readSession.close(() => { + expect(result.records.map(record => record.get(0))).toEqual(['Bob', 'Alice', 'Tina']); + + setupFakeHostNameResolution(driver, '127.0.0.1:9010', ['127.0.0.1:9020']); + + const writeSession = driver.session(WRITE); + writeSession.run('CREATE (n {name:\'Bob\'})').then(result => { + writeSession.close(() => { + expect(result.records).toEqual([]); + + driver.close(); + + seedRouter.exit(code1 => { + resolvedSeedRouter.exit(code2 => { + reader.exit(code3 => { + writer.exit(code4 => { + expect(code1).toEqual(0); + expect(code2).toEqual(0); + expect(code3).toEqual(0); + expect(code4).toEqual(0); + done(); + }); + }); + }); + }); + }); + }); + }); + }); + }); + }); + function moveNextDateNow30SecondsForward() { const currentTime = Date.now(); hijackNextDateNowCall(currentTime + 30 * 1000 + 1); From cf431063ee76cdcda9ef6db11f5dde2ff952c4ed Mon Sep 17 00:00:00 2001 From: lutovich Date: Thu, 27 Apr 2017 17:24:38 +0200 Subject: [PATCH 2/2] Couple fixes after code review --- src/v1/index.js | 2 +- test/internal/connector.test.js | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/v1/index.js b/src/v1/index.js index 6103b7b23..948259443 100644 --- a/src/v1/index.js +++ b/src/v1/index.js @@ -132,7 +132,7 @@ function driver(url, authToken, config = {}) { return new RoutingDriver(parseUrl(url), routingContext, USER_AGENT, authToken, config); } else if (scheme === 'bolt://') { if (!isEmptyObjectOrNull(routingContext)) { - throw new Error(`Routing parameters are not supported with scheme 'bolt'. Given URL: '${url}'`); + throw new Error(`Parameters are not supported with scheme 'bolt'. Given URL: '${url}'`); } return new Driver(parseUrl(url), USER_AGENT, authToken, config); } else { diff --git a/test/internal/connector.test.js b/test/internal/connector.test.js index ba29ec2c4..6d9ba1091 100644 --- a/test/internal/connector.test.js +++ b/test/internal/connector.test.js @@ -132,9 +132,7 @@ describe('connector', () => { it('should notify when connection initialization fails', done => { const connection = connect('bolt://localhost:7474'); // wrong port - connection.initializationCompleted().then(() => { - console.log('THEN called: ', arguments) - }).catch(error => { + connection.initializationCompleted().catch(error => { expect(error).toBeDefined(); done(); });