SWIFT-742 Implement unified replica set discovery behavior #490
Conversation
| let hostAddress = try ServerAddress(host) | ||
|
|
||
| // check event count and that events are of the expected types | ||
| expect(receivedEvents.count).to(beGreaterThanOrEqualTo(5)) |
There was a problem hiding this comment.
in this test we previously saw:
- A topology opening event
- A topology changed event (change from unknown to single, no hosts in list)
- A server opening event
- A server changed event, from unknown to standalone
- A topology changed event, adding in the new server to the topology
We now see:
- A topology opening event
- A server opening event
- A server changed event, from unknown to standalone
- A topology changed event, from unknown to single
Previously in this case libmongoc by default would behave like the directConnection=true option was specified, which meant the topology would be assumed to be single right off the bat before discovering servers. Now that we specify directConnection=false that transition doesn't happen until the server has been discovered.
Codecov Report
@@ Coverage Diff @@
## master #490 +/- ##
==========================================
- Coverage 76.98% 76.79% -0.19%
==========================================
Files 125 125
Lines 13050 13080 +30
==========================================
- Hits 10046 10045 -1
- Misses 3004 3035 +31
Continue to review full report at Codecov.
|
patrickfreed
left a comment
patrickfreed
left a comment
There was a problem hiding this comment.
lgtm. I think in the commit message / ticket / release notes we should mention the behavioral change that came from this, since it is kind of hard to surmise it from the ticket name.
patrickfreed
left a comment
patrickfreed
left a comment
There was a problem hiding this comment.
I wonder if it's worth adding a test that verifies this option works, something like:
- only run against replica sets
- split up URI into separate hosts
- connect to each of them separately (with
directConnectionunspecified) and try to perform an insert - assert insert worked (meaning each discovered the primary)
Possibly repeat the test with directConnection=true and verify at least one of the attempts failed.
|
whoops sorry I missed adding you somehow @nbbeeken. I will take a crack at adding a test like that now @patrickfreed |
|
test added; was actually very easy thanks to the |
patrickfreed
left a comment
patrickfreed
left a comment
There was a problem hiding this comment.
test looks good, one doc suggestion, and I think this needs to be rebased off of master to get the error changes
Sources/MongoSwift/MongoClient.swift
Outdated
| public var dateCodingStrategy: DateCodingStrategy? | ||
|
|
||
| /// When true, the client will connect directly to a single host. When false, the client will attempt to | ||
| /// automatically discover all replica set members if a replica set name is provided. |
There was a problem hiding this comment.
I think we should mention that the default behavior is the false one.
There was a problem hiding this comment.
whoops, I had that in here but it seems like I accidentally deleted it, good catch. added
kmahar
force-pushed
the
SWIFT-742/unified-repl-set-discovery
branch
from
33463d9 to
1e4cfaa
Compare
This builds on my previous PR to add libmongoc support for
directConnection=trueto give us the desired discover behavior by default.You can view a summary of the spec changes here.
I was able to pull in the relevant initial DNS seedlist discovery tests as we implement that test runner, but the other tests use the SDAM and connection string test runners which we do not implement yet.
I did some manual local testing using SDAM monitoring and everything seems to work as expected in terms of initial topology assumptions as well as errors being reported in the right scenarios.