-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Support per-MongoClient DNS lookup configuration #1104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
79ab874
fa1a01f
2edd337
a89c2b3
37885d2
0f34214
088d3cd
c0b99c5
47e665b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looks like we have been throwing this exception to users despite it not being There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We generally need them to be public for the reason mentioned. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [optional]
Being
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
super(-2, msg, e); | ||
this.serverAddress = serverAddress; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unfortunate but I don't see a way around it, as ConnectionString is responsible for TXT record resolution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toMongoClient#create
. If you then try to apply the TXT record to theMongoClientSettings
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.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, aMongoClientSettings
/MongoClientSettings.Builder
that was fed aConnectionString
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 aMongoClientSettings
/Builder
, he may pack that data in any form needed together withMongoClientSettings
/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 aConnectionString
(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;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. Apossible solution(not really):ConnectionString
(and any other settings that require IO or customizable logic)" mentioned previously may even be theMongoClientSettings.build
method, as at this point we have all the user customizations available to us. I don't like it, but seems legit. Thebuild
method may even have an async version (unlike a constructor ofConnestionString
), but ourDnsClient
andInetAddressResolver
a synchronous anyway...ConnectionString
to aBuilder
, rather we only allow creating a newBuilder
from a previously built (and, therefore, fully resolved)MongoClientSettings
with aConnectionString
that overrides some of the settings.applyConnectionString
elements.MongoClientSettings
can now provide getters.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. OurMongoClients.create(String)
is a constructor of a client, and it does IO, which violates the SDAM requirement. Even if we had onlyMongoClients.create(ConnectionString)
instead, it would still violate the requirement since creation of aConnectionString
is effectively part of the client's construction.P.S. This also makes it obvious that the idea of resolving a
ConnectionString
inMongoClientSettings.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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created https://jira.mongodb.org/browse/JAVA-4944.