feat: support load balancer connections COMPASS-4898 #2326
Conversation
|
I'm taking ideas for how to add tests here, if anybody has any :) |
addaleax
changed the title
Pending a driver update to include NODE-3011, this should give us what we need to support load balanced connections (specifically, I tested this against the current state of that PR and it works).
| @@ -1046,14 +1048,23 @@ Connection = AmpersandModel.extend({ | |||
| const parseConnectionStringAsPromise = promisify(parseConnectionString); | |||
|
|
|||
| async function createConnectionFromUrl(url) { | |||
| // We use resolveMongodbSrv because it understands the load balancer | |||
| // option, whereas parseConnectionString from the 3.6 driver does not. | |||
| // This could potentially go away once we're using the 3.7 driver, | |||
There was a problem hiding this comment.
// This could potentially go away once we're using the 3.7 driver,
4.x would work directly with this right?
There was a problem hiding this comment.
There is no parseOptions like the one we use here in the 4.x driver, otherwise we would use it :)
| @@ -1046,14 +1048,23 @@ Connection = AmpersandModel.extend({ | |||
| const parseConnectionStringAsPromise = promisify(parseConnectionString); | |||
|
|
|||
| async function createConnectionFromUrl(url) { | |||
| // We use resolveMongodbSrv because it understands the load balancer | |||
There was a problem hiding this comment.
What was happening with "mongo3" here, did we get an error or just the loadBalanced=true was not added?
There was a problem hiding this comment.
Yeah, we get an error because it rejects the unknown option.
There was a problem hiding this comment.
The SRV resolution we get in Compass is a side effect of a driver change in the parseConnectionString (when srv+mongodb support has been added) and causes more chaos than what's useful for.
If the old parseConnectionString just don’t understand loadBalanced but don’t error either, maybe we could keep everything as it is for now, at least in the meanwhile we are debugging the other connectivity issues.
Or … since we finally are changing this parseConnectionString to ConnectionString maybe is also time to drop it entirely, if @mmarcon is ok with that.
|
I don't have enough context to recommend one option vs the other. Please choose the option you think is best. |
@mmarcon If we don't parse SRV the list of host in the sidebar won't be populated with the actual hostnames for srv records. If that's important maybe we should keep the resolution for now and we could just move it to a place that does not interfere with the connection later. Though that's something we may want to do in future with a bit more calm. |
Err yeah, I think we probably should, thanks for pointing it out 👍
Yeah, I think we actually should skip displaying the actual hostnames here – but I’d probably leave that for when we move our usage of the 3.x driver option parsing. |
Yeah that's a good point, probably the format is very different. What was happening before we did this? |
Yes. |
@mcasimir I think it's an acceptable compromise. Let's make a ticket to bring it back in the near future. |
|
This is how the sidebar looks now:
|
Pending a driver update to include NODE-3011, this should give
us what we need to support load balanced connections (specifically,
I tested this against the current state of that PR and it works).
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes