Support per-MongoClient DNS lookup configuration #1104

jyemin · 2023-04-07T14:10:19Z

Add InetAddressResolver interface
Add configuration of InetAddressResolver and DnsClient in MongoClientSettings and ConnectionString
Use configured InetAddressResolver and DnsClient to resolve all SRV, TXT, A, and AAAA records

* Add InetAddressResolver interface * Add configuration of InetAddressResolver and DnsClient in MongoClientSettings and ConnectionString * Use configured InetAddressResolver and DnsClient to resolve all SRV, TXT, A, and AAAA records JAVA-4911

jyemin · 2023-04-07T14:10:51Z

driver-core/src/main/com/mongodb/ConnectionString.java

+    /**
+     * Creates a ConnectionString from the given string with the given {@link DnsClient}.
+     *
+     * <p>If setting {@link MongoClientSettings#getDnsClient()} explicitly, care should be taken to call this constructor with the same


This is unfortunate but I don't see a way around it, as ConnectionString is responsible for TXT record resolution.

[single responsibility] This is where packing configuration together with the logic of processing it together in a public class bit us.

Do we want a ticket about refactoring ConnectionString in 5.0?

I just can't think of a concrete plan for it, even if we allow breaking changes. What happens if you call any of the applyConnectioString methods on the setting classes, and then pass a MongoClientSettings in to MongoClient#create. If you then try to apply the TXT record to the MongoClientSettings you have the problem that the settings are supposed to trump the TXT record and so you have to remember in the settings the difference between a value that is the default and the same value that was specified explicitly. Tricky.

On getters

A ConnectionString that does not do what it's not supposed to do should verify the format of the string, may destructure it on components, but should not provide any getters to users (even if currently implementing most of them don't require any IO, who knows what may happen in the future). Therefore, a MongoClientSettings/MongoClientSettings.Builder that was fed a ConnectionString should not provide any getters to users either. I don't think these getters are actually required by users: even if in some bizarre cases a user wants to know what's inside a MongoClientSettings/Builder, he may pack that data in any form needed together with MongoClientSettings/Builder in another structure (a weird requirement needs some additional work on the user side, and is not supported out of the box, that's fine).

If for some reason we end up strongly wanting to provide getters (I hope we don't), we may have two sets of MongoClientSettings/Builder types: as long as a ConnectionString (or any other future mechanism that requires IO or customizable logic similarly to the +srv connection string modifier) is not used, we use the types with getters, otherwise we return a type without getters (see the next section for more details).

On the path forward

[As soon as I posted the first attempt, I realized that the two paths I described are actually different ways of implementing the same approach, so I rewrote this section.]

We may maintain a sequence of updates in MongoClientSettings/Builder:

applyConnectionString introduces a new element to the sequence;

any other setter is merged together with the immediate previous update as long as that update is not applyConnectionString, otherwise it also introduces a new element to the sequence.
At the point when we are ready to resolve ConnectionString (and any other settings that require IO or customizable logic), we resolve and merge the updates in the sequence.

The open question here is how to avoid doing multiple rounds of IO due to having multiple applyConnectionString elements (with +srv or something similar) in the sequence. A ~~possible solution~~ (not really):

We notice that the "point when we are ready to resolve ConnectionString (and any other settings that require IO or customizable logic)" mentioned previously may even be the MongoClientSettings.build method, as at this point we have all the user customizations available to us. I don't like it, but seems legit. The build method may even have an async version (unlike a constructor of ConnestionString), but our DnsClient and InetAddressResolver a synchronous anyway...

We don't allow applying a ConnectionString to a Builder, rather we only allow creating a new Builder from a previously built (and, therefore, fully resolved) MongoClientSettings with a ConnectionString that overrides some of the settings.

Now we have at most two elements in the sequence, and we never need to do multiple rounds of IO when resolving settings as there are never multiple applyConnectionString elements.

As a side bonus (if one may call it a bonus), MongoClientSettings can now provide getters.

[optional]

Does the above seem like something we could explore further when designing the 5.0 API improvements? If yes, I'll create a ticket. If not, or if we don't really want to have any significant API changes in 5.0, then I won't.

I went to such length in this discussion because the requirement If setting {@link MongoClientSettings#getDnsClient()} explicitly, care should be taken to call this constructor with the same {@link DnsClient} screams at me that we failed at the API design here.

I found something related in the SDAM spec https://github.com/mongodb/specifications/blob/master/source/server-discovery-and-monitoring/server-discovery-and-monitoring-summary.rst:

"
A multi-threaded or asynchronous client's constructor MUST NOT do any I/O. ... The justification is that if clients can be constructed when the deployment is in some states but not in other states, it leads to an unfortunate scenario: When the deployment is passing through a strange state, long-running applications may keep working, but any applications restarted during this period fail.
...
Single-threaded clients also MUST NOT do I/O in the constructor. ...
"

Thile the SDAM spec focuses on specifically client constructor and the state of the MongoDB deployment, the principle of not doing IO in constructors should always be followed for the same reason pointed out in the spec: object creation should not fail because of the temporary unfortunate external state. The constructor of ConnectionString may fail due not network issues, which may result in the whole application failing to start. Our MongoClients.create(String) is a constructor of a client, and it does IO, which violates the SDAM requirement. Even if we had only MongoClients.create(ConnectionString) instead, it would still violate the requirement since creation of a ConnectionString is effectively part of the client's construction.

P.S. This also makes it obvious that the idea of resolving a ConnectionString in MongoClientSettings.build that I expressed above was really bad, and the question on "how to avoid doing multiple rounds of IO due to having multiple applyConnectionString elements (with +srv or something similar) in the sequence" stays very much open. We may talk with other driver teams to see what they do, if we decide to explore this for 5.0.

Maybe what was bad was supporting TXT records. :). I can only say that we knew at the time we were violating the SDAM spec recommendation but didn't see a lot of good options given the API we had to support at the time.

Created https://jira.mongodb.org/browse/JAVA-4944.

driver-core/src/main/com/mongodb/internal/connection/ServerAddressWithResolver.java

driver-core/src/main/com/mongodb/internal/dns/DefaultDnsResolver.java

stIncMale

I have not yet reviewed the whole PR.

driver-core/src/main/com/mongodb/internal/connection/ServerAddressWithResolver.java

driver-core/src/main/com/mongodb/spi/dns/InetAddressResolver.java

driver-core/src/main/com/mongodb/internal/connection/ServerAddressWithResolver.java

driver-core/src/main/com/mongodb/spi/dns/InetAddressResolver.java

driver-core/src/main/com/mongodb/spi/dns/InetAddressResolverProvider.java

driver-core/src/main/com/mongodb/spi/dns/InetAddressResolver.java

stIncMale · 2023-04-13T14:13:29Z

driver-core/src/main/com/mongodb/MongoSocketException.java

@@ -33,7 +33,7 @@ public class MongoSocketException extends MongoException {
     * @param msg the message
     * @param e the cause
     */
-    MongoSocketException(final String msg, final ServerAddress serverAddress, final Throwable e) {
+    public MongoSocketException(final String msg, final ServerAddress serverAddress, final Throwable e) {


I looks like we have been throwing this exception to users despite it not being public for a long time. Was it wrong, and MongoSocketException was meant to be public all that time?

The class is public, it's just the constructor that was not. Typically the constructors for exceptions are public, if only because they often need to be called from class in the driver that are in a different package.

Sorry, I was apparently temporarily blinded at the time of writing the first comment. Why do we need to expose constructors of public exceptions in the public API?

There was a situation when I had to make a constructor public (MongoConnectionPoolClearedException), but I explicitly stated that the constructor is not part of the public API.

We generally need them to be public for the reason mentioned. The VisibleForTesting annotation is not really accurate, but I suppose we could use it for constructors that we don't otherwise want to be public.

[optional]

We generally need them to be public for the reason mentioned.

Being public and being part of the public API is not the same. In this case, the constructor has to be public, but, as far as I can judge from this conversation, does not have to be part of the public API.

The VisibleForTesting annotation is not really accurate

I agree, it should not be used here. But we can document the constructor as not being part of the public API, similarly to how the constructor of MongoConnectionPoolClearedException is documented. By doing so, we reduce the API surface. Do you see reasons why this is a bad idea?

There's just a lack of consistency in that the large majority of our exception class constructors are already part of the API and there is not a very clear path to changing that (would we deprecate them all?). So making this one not part of the API doesn't help us all that much. I wouldn't be against a ticket that considers options for shrinking out total constructor API.

Created https://jira.mongodb.org/browse/JAVA-4942 and https://jira.mongodb.org/browse/JAVA-4943.

stIncMale · 2023-04-13T16:33:27Z

driver-core/src/main/com/mongodb/ConnectionString.java

+    /**
+     * Creates a ConnectionString from the given string with the given {@link DnsClient}.
+     *
+     * <p>If setting {@link MongoClientSettings#getDnsClient()} explicitly, care should be taken to call this constructor with the same


[single responsibility] This is where packing configuration together with the logic of processing it together in a public class bit us.

Do we want a ticket about refactoring ConnectionString in 5.0?

driver-core/src/main/com/mongodb/MongoClientSettings.java

driver-sync/src/test/functional/com/mongodb/client/AbstractDnsConfigurationTest.java

stIncMale

.

stIncMale

Approved, with two optional things:

jyemin commented Apr 7, 2023

View reviewed changes

Fix Javadoc

fa1a01f

jyemin requested review from stIncMale and katcharov April 7, 2023 14:22

stIncMale requested changes Apr 12, 2023

View reviewed changes

stIncMale requested changes Apr 13, 2023

View reviewed changes

Code review updates

2edd337

jyemin requested a review from stIncMale April 13, 2023 20:33

stIncMale requested changes Apr 14, 2023

View reviewed changes

jyemin added 5 commits April 14, 2023 14:16

Added ThreadSafe annotations

a89c2b3

linkplain

37885d2

ThreadSafe take two

0f34214

Removed unnecessary documentation

088d3cd

Add IP literal check. Fix Javadoc

c0b99c5

jyemin requested a review from stIncMale April 14, 2023 18:47

stIncMale approved these changes Apr 17, 2023

View reviewed changes

Fix test

47e665b

jyemin merged commit a43c2cf into mongodb:master Apr 18, 2023

jyemin deleted the j4911 branch April 18, 2023 16:03

Support per-MongoClient DNS lookup configuration #1104

Support per-MongoClient DNS lookup configuration #1104

Uh oh!

Conversation

jyemin commented Apr 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stIncMale Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

On getters

On the path forward

Uh oh!

stIncMale Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stIncMale Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stIncMale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stIncMale Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stIncMale left a comment

Choose a reason for hiding this comment

Uh oh!

stIncMale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stIncMale Apr 14, 2023 •

edited

Loading

stIncMale Apr 17, 2023 •

edited

Loading

stIncMale Apr 18, 2023 •

edited

Loading

stIncMale Apr 17, 2023 •

edited

Loading