fix(data-service): remove compass connectivity check via ping COMPASS-9455#7897
fix(data-service): remove compass connectivity check via ping COMPASS-9455#7897
Conversation
nbbeeken
requested review from
Copilot and
lerouxb
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
Removes the explicit post-connect ping round-trip in data-service so connections rely on the driver’s connect() verification, reducing one RTT during connection establishment.
Changes:
- Removed the
runCommand(..., { ping: 1 })connectivity check after creating a client. - Updated the test suite by removing the ping/readPreference-specific assertion and associated type import cleanup.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/data-service/src/connect-mongo-client.ts | Drops the explicit ping command executed right after connectMongoClient() completes. |
| packages/data-service/src/connect-mongo-client.spec.ts | Removes a ping-specific test and cleans up now-unused MongoDB event typing. |
|
|
||
| it('should not run the ping command with the specified ReadPreference', async function () { | ||
| const connectionString = clusterConnectionStringURL.clone(); | ||
| connectionString | ||
| .typedSearchParams<MongoClientOptions>() | ||
| .set('readPreference', 'secondaryPreferred'); | ||
| const commands: CommandStartedEvent[] = []; | ||
| const [metadataClient, crudClient, , state] = await connectMongoClient({ | ||
| connectionOptions: { | ||
| connectionString: connectionString.toString(), | ||
| }, | ||
| setupListeners: (client) => | ||
| client.on('commandStarted', (ev) => commands.push(ev)), | ||
| }); | ||
| expect(commands).to.have.lengthOf(1); | ||
| expect(commands[0].commandName).to.equal('ping'); | ||
| expect(commands[0].command.$readPreference).to.equal(undefined); | ||
|
|
||
| for (const closeLater of [metadataClient, crudClient, state]) { | ||
| toBeClosed.add(closeLater); | ||
| } | ||
| }); | ||
|
|
||
| describe('ssh tunnel failures', function () { | ||
| // Use async_hooks to track the state of the internal network server used |
There was a problem hiding this comment.
The PR removes the ping-on-connect behavior but also removes the only test that asserted a ping command was issued. To prevent regressions (reintroducing the extra RTT), consider replacing it with a test that asserts no ping command is emitted during connectMongoClient (e.g., via commandStarted events and checking that none have commandName === 'ping').
gribnoysup
left a comment
gribnoysup
left a comment
There was a problem hiding this comment.
Not a bad suggestion to add a new test for this, otherwise lgtm
|
Depends on: mongodb-js/devtools-shared#628 |
Description
We now use a driver version that does connection checkout/in verifying connectivity and auth in the connect method!! -1 RTT. huge!
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes