Skip to content

Commit

Permalink
fix(shell-api): warn instead of throw for sh methods on non-mongos MO…
Browse files Browse the repository at this point in the history 
…NGOSH-982 (#1098)

Also cache the `db.hello()` result instead of running it for
every single `sh` method call.

Testing the extended `sh.status(verbose, db)` signature is left
for MONGOSH-742.
  • Loading branch information
@addaleax
addaleax authored Sep 10, 2021
1 parent e2811dd commit 6133645
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 80 deletions.
16 changes: 12 additions & 4 deletions packages/shell-api/src/database.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ import {
getPrintableShardStatus,
processDigestPassword,
tsToSeconds,
isValidCollectionName
isValidCollectionName,
getConfigDB
} from './helpers';

import type {
Expand Down Expand Up @@ -59,6 +60,7 @@ export default class Database extends ShellApiWithMongoClass {
_collections: Record<string, Collection>;
_session: Session | undefined;
_cachedCollectionNames: string[] = [];
_cachedHello: Document | null = null;

constructor(mongo: Mongo, name: string, session?: Session) {
super();
Expand Down Expand Up @@ -103,6 +105,10 @@ export default class Database extends ShellApiWithMongoClass {
return options;
}

async _maybeCachedHello(): Promise<Document> {
return this._cachedHello ?? await this.hello();
}

/**
* Internal method to determine what is printed for this class.
*/
Expand Down Expand Up @@ -891,16 +897,18 @@ export default class Database extends ShellApiWithMongoClass {
async hello(): Promise<Document> {
this._emitDatabaseApiCall('hello', {});
try {
return await this._runCommand(
this._cachedHello = await this._runCommand(
{
hello: 1,
}
);
return this._cachedHello;
} catch (err) {
if (err.codeName === 'CommandNotFound') {
const result = await this.isMaster();
delete result.ismaster;
return result;
this._cachedHello = result;
return this._cachedHello;
}
throw err;
}
Expand Down Expand Up @@ -1248,7 +1256,7 @@ export default class Database extends ShellApiWithMongoClass {
@apiVersions([1])
async printShardingStatus(verbose = false): Promise<CommandResult> {
this._emitDatabaseApiCall('printShardingStatus', { verbose });
const result = await getPrintableShardStatus(this, verbose);
const result = await getPrintableShardStatus(await getConfigDB(this), verbose);
return new CommandResult('StatsResult', result);
}

Expand Down
31 changes: 11 additions & 20 deletions packages/shell-api/src/helpers.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,23 +76,14 @@ describe('getPrintableShardStatus', () => {
const testServer = startTestServer('shared');

let mongo: Mongo;
let database: Database;
let configDatabase: Database;
let serviceProvider: ServiceProvider;
let inBalancerRound = false;

beforeEach(async() => {
serviceProvider = await CliServiceProvider.connect(await testServer.connectionString(), {}, {}, new EventEmitter());
mongo = new Mongo(new ShellInstanceState(serviceProvider), undefined, undefined, undefined, serviceProvider);
database = new Database(mongo, 'db1');
const origGetSiblingDB = database.getSiblingDB;
database.getSiblingDB = (dbname) => {
if (dbname === 'config') {
dbname = 'config_test';
}
return origGetSiblingDB.call(database, dbname);
};
configDatabase = database.getSiblingDB('config');
configDatabase = new Database(mongo, 'config_test');
expect(configDatabase.getName()).to.equal('config_test');

const origRunCommandWithCheck = serviceProvider.runCommandWithCheck;
Expand Down Expand Up @@ -125,7 +116,7 @@ describe('getPrintableShardStatus', () => {
});

it('returns an object with sharding information', async() => {
const status = await getPrintableShardStatus(database, false);
const status = await getPrintableShardStatus(configDatabase, false);
expect(status.shardingVersion.currentVersion).to.be.a('number');
expect(status.shards.map(({ host }) => host)).to.include('shard01/localhost:27018,localhost:27019,localhost:27020');
expect(status['most recently active mongoses']).to.have.lengthOf(1);
Expand All @@ -140,19 +131,19 @@ describe('getPrintableShardStatus', () => {
it('returns whether the balancer is currently running', async() => {
{
inBalancerRound = true;
const status = await getPrintableShardStatus(database, true);
const status = await getPrintableShardStatus(configDatabase, true);
expect(status.balancer['Currently running']).to.equal('yes');
}

{
inBalancerRound = false;
const status = await getPrintableShardStatus(database, true);
const status = await getPrintableShardStatus(configDatabase, true);
expect(status.balancer['Currently running']).to.equal('no');
}
});

it('returns an object with verbose sharding information if requested', async() => {
const status = await getPrintableShardStatus(database, true);
const status = await getPrintableShardStatus(configDatabase, true);
expect(status['most recently active mongoses'][0].up).to.be.a('number');
expect(status['most recently active mongoses'][0].waiting).to.be.a('boolean');
});
Expand All @@ -162,7 +153,7 @@ describe('getPrintableShardStatus', () => {
_id: 'balancer',
activeWindow: { start: '00:00', stop: '23:59' }
});
const status = await getPrintableShardStatus(database, false);
const status = await getPrintableShardStatus(configDatabase, false);
expect(status.balancer['Balancer active window is set between'])
.to.equal('00:00 and 23:59 server local time');
});
Expand All @@ -177,7 +168,7 @@ describe('getPrintableShardStatus', () => {
what: 'balancer.round',
ns: ''
});
const status = await getPrintableShardStatus(database, false);
const status = await getPrintableShardStatus(configDatabase, false);
expect(status.balancer['Failed balancer rounds in last 5 attempts']).to.equal(1);
expect(status.balancer['Last reported error']).to.equal('Some error');
});
Expand All @@ -189,7 +180,7 @@ describe('getPrintableShardStatus', () => {
ts: new bson.ObjectId('5fce116c579db766a198a176'),
when: new Date('2020-12-07T11:26:36.803Z'),
});
const status = await getPrintableShardStatus(database, false);
const status = await getPrintableShardStatus(configDatabase, false);
expect(status.balancer['Collections with active migrations']).to.have.lengthOf(1);
expect(status.balancer['Collections with active migrations'].join('')).to.include('asdf');
});
Expand All @@ -200,7 +191,7 @@ describe('getPrintableShardStatus', () => {
what: 'moveChunk.from',
details: { from: 'shard0', to: 'shard1', note: 'success' }
});
const status = await getPrintableShardStatus(database, false);
const status = await getPrintableShardStatus(configDatabase, false);
expect(status.balancer['Migration Results for the last 24 hours'])
.to.deep.equal({ 1: 'Success' });
});
Expand All @@ -211,7 +202,7 @@ describe('getPrintableShardStatus', () => {
what: 'moveChunk.from',
details: { from: 'shard0', to: 'shard1', errmsg: 'oopsie' }
});
const status = await getPrintableShardStatus(database, false);
const status = await getPrintableShardStatus(configDatabase, false);

expect(status.balancer['Migration Results for the last 24 hours'])
.to.deep.equal({ 1: "Failed with error 'oopsie', from shard0 to shard1" });
Expand All @@ -220,7 +211,7 @@ describe('getPrintableShardStatus', () => {
it('fails when config.version is empty', async() => {
await configDatabase.getCollection('version').drop();
try {
await getPrintableShardStatus(database, false);
await getPrintableShardStatus(configDatabase, false);
} catch (err) {
expect(err.name).to.equal('MongoshInvalidInputError');
return;
Expand Down
13 changes: 6 additions & 7 deletions packages/shell-api/src/helpers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,12 +170,10 @@ export function processDigestPassword(
* @param configDB
* @param verbose
*/
export async function getPrintableShardStatus(db: Database, verbose: boolean): Promise<Document> {
const result = {} as any; // use array to maintain order
export async function getPrintableShardStatus(configDB: Database, verbose: boolean): Promise<Document> {
const result = {} as any;

// configDB is a DB object that contains the sharding metadata of interest.
// Defaults to the db named "config" on the current connection.
const configDB = await getConfigDB(db);
const mongosColl = configDB.getCollection('mongos');
const versionColl = configDB.getCollection('version');
const shardsColl = configDB.getCollection('shards');
Expand Down Expand Up @@ -292,7 +290,7 @@ export async function getPrintableShardStatus(db: Database, verbose: boolean): P
versionHasActionlog = true;
}
if (metaDataVersion === 5) {
const verArray = (await db.serverBuildInfo()).versionArray;
const verArray = (await configDB.serverBuildInfo()).versionArray;
if (verArray[0] === 2 && verArray[1] > 6) {
versionHasActionlog = true;
}
Expand Down Expand Up @@ -484,9 +482,10 @@ export async function getPrintableShardStatus(db: Database, verbose: boolean): P
}

export async function getConfigDB(db: Database): Promise<Database> {
const helloResult = await db.hello();
const helloResult = await db._maybeCachedHello();
if (helloResult.msg !== 'isdbgrid') {
throw new MongoshInvalidInputError('Not connected to a mongos', ShellApiErrors.NotConnectedToMongos);
await db._instanceState.printWarning(
'MongoshWarning: [SHAPI-10003] You are not connected to a mongos. This command may not work as expected.');
}
return db.getSiblingDB('config');
}
Expand Down
Loading

0 comments on commit 6133645

Please sign in to comment.