[JAVA-6060] Reduce global state by grouping internal settings#1864
[JAVA-6060] Reduce global state by grouping internal settings#1864nhachicha wants to merge 3 commits intomongodb:mainfrom
Conversation
…ntSettings configuration
There was a problem hiding this comment.
Pull request overview
This pull request refactors internal MongoDB client configuration by replacing static global state with instance-based settings. The main change centralizes internal settings into a new InternalMongoClientSettings class that encapsulates both connection pool settings and the recordEverything flag, which was previously a static variable in InternalStreamConnection.
Changes:
- Introduced
InternalMongoClientSettingsclass to encapsulate internal configuration includingInternalConnectionPoolSettingsandrecordEverythingflag - Created
InternalMongoClientsfactory classes for both sync and reactive drivers to handle client creation with internal settings - Removed static
recordEverythingfield fromInternalStreamConnectionin favor of instance-based configuration - Updated test classes to use new factory methods and pass internal settings explicitly instead of relying on global state
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| driver-core/src/main/com/mongodb/internal/connection/InternalMongoClientSettings.java | New immutable settings class containing internal connection pool settings and recordEverything flag |
| driver-core/src/main/com/mongodb/internal/connection/InternalStreamConnection.java | Removed static recordEverything field and added instance field; updated constructors to accept recordEverything parameter |
| driver-core/src/main/com/mongodb/internal/connection/InternalStreamConnectionFactory.java | Added recordEverything parameter to constructors and passed it to InternalStreamConnection |
| driver-core/src/main/com/mongodb/internal/connection/DefaultClusterFactory.java | Updated to use InternalMongoClientSettings instead of InternalConnectionPoolSettings |
| driver-core/src/main/com/mongodb/internal/connection/DefaultClusterableServerFactory.java | Replaced InternalConnectionPoolSettings parameter with InternalMongoClientSettings |
| driver-core/src/main/com/mongodb/internal/connection/LoadBalancedClusterableServerFactory.java | Replaced InternalConnectionPoolSettings parameter with InternalMongoClientSettings |
| driver-sync/src/main/com/mongodb/client/MongoClients.java | Refactored to delegate to InternalMongoClients with default internal settings |
| driver-sync/src/main/com/mongodb/client/internal/InternalMongoClients.java | New factory class for creating MongoClient instances with custom internal settings |
| driver-sync/src/main/com/mongodb/client/internal/Clusters.java | Added overload accepting InternalMongoClientSettings |
| driver-sync/src/main/com/mongodb/client/internal/MongoClientImpl.java | Removed createCluster and getStreamFactory helper methods (moved to InternalMongoClients) |
| driver-reactive-streams/src/main/com/mongodb/reactivestreams/client/MongoClients.java | Refactored to delegate to InternalMongoClients with default internal settings |
| driver-reactive-streams/src/main/com/mongodb/reactivestreams/client/internal/InternalMongoClients.java | New factory class for creating reactive MongoClient instances with custom internal settings |
| driver-sync/src/test/functional/com/mongodb/client/AbstractClientSideOperationsTimeoutProseTest.java | Added createMongoClientWithInternalSettings method to support tests requiring internal settings |
| driver-sync/src/test/functional/com/mongodb/client/ClientSideOperationTimeoutProseTest.java | Implemented createMongoClientWithInternalSettings method |
| driver-sync/src/test/functional/com/mongodb/client/AbstractClientMetadataProseTest.java | Removed static recordEverything setup/teardown |
| driver-sync/src/test/functional/com/mongodb/client/ClientMetadataProseTest.java | Updated to use InternalMongoClients with recordEverything enabled |
| driver-sync/src/test/functional/com/mongodb/internal/connection/OidcAuthenticationProseTests.java | Updated to use InternalMongoClients with recordEverything enabled; removed static setup/teardown |
| driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/ClientSideOperationTimeoutProseTest.java | Implemented createMongoClientWithInternalSettings method for reactive client |
| driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/AbstractClientMetadataProseTest.java | Updated to use InternalMongoClients with recordEverything enabled |
| driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/syncadapter/SyncMongoClient.java | Added constructor accepting reactive MongoClient for wrapping |
| driver-sync/src/test/unit/com/mongodb/client/internal/InternalMongoClientsTest.java | New unit tests for InternalMongoClients functionality |
| driver-reactive-streams/src/test/unit/com/mongodb/reactivestreams/client/internal/InternalMongoClientsTest.java | New unit tests for reactive InternalMongoClients |
| driver-core/src/test/functional/com/mongodb/internal/connection/TestCommandListener.java | Updated javadoc reference from static method to builder method |
| driver-core/src/test/functional/com/mongodb/internal/connection/SingleServerClusterTest.java | Updated to use InternalMongoClientSettings.getDefaults() |
| driver-core/src/test/functional/com/mongodb/internal/connection/CommandHelperSpecification.groovy | Updated to pass recordEverything parameter to InternalStreamConnectionFactory |
| driver-core/src/test/functional/com/mongodb/ClusterFixture.java | Updated to use InternalMongoClientSettings.getDefaults() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
driver-sync/src/test/unit/com/mongodb/client/internal/InternalMongoClientsTest.java
Outdated
Show resolved
Hide resolved
…MongoClientsTest.java Co-authored-by: Copilot <[email protected]>
rozza
left a comment
There was a problem hiding this comment.
A couple of nits and a couple of questions.
Looking much cleaner though.
| * Used for testing and internal configuration purposes. | ||
| * | ||
| * <p>This class is not part of the public API and may be removed or changed at any time</p> | ||
| */ |
There was a problem hiding this comment.
| */ | |
| * @since 5.7 | |
| */ |
| * | ||
| * @return the default settings | ||
| */ | ||
| public static InternalMongoClientSettings getDefaults() { |
There was a problem hiding this comment.
I think this might be a new pattern, so I'm hesitant to add it.
As its an internal class and its immutable just make the static public. Thats the pattern we use with TimeoutSettings.DEFAULT.
| @Immutable | ||
| public final class InternalMongoClientSettings { | ||
|
|
||
| private static final InternalMongoClientSettings DEFAULTS = builder().build(); |
There was a problem hiding this comment.
See comment below regarding #getDefaults
| private static final InternalMongoClientSettings DEFAULTS = builder().build(); | |
| public static final InternalMongoClientSettings DEFAULT = builder().build(); |
| * Internal factory for MongoClient instances. | ||
| * | ||
| * <p>This class is not part of the public API and may be removed or changed at any time</p> | ||
| */ |
There was a problem hiding this comment.
| */ | |
| * @since 5.7 | |
| */ |
| */ | ||
| public final class MongoClients { | ||
|
|
||
| private static final InternalMongoClientSettings DEFAULT_INTERNAL_SETTINGS = InternalMongoClientSettings.getDefaults(); |
There was a problem hiding this comment.
Looks like we could just use this in InternalMongoClients and keep usage in the internal namespace.
| * @param reactiveMongoClient the reactive MongoClient to wrap | ||
| */ | ||
| public SyncMongoClient(final com.mongodb.reactivestreams.client.MongoClient reactiveMongoClient) { | ||
| this.connectionPoolCounter = new ConnectionPoolCounter(); |
There was a problem hiding this comment.
This doesn't ensure the connectionPoolCounter is applied to the connection pool listener. Could that be an issue?
| */ | ||
| public final class MongoClients { | ||
|
|
||
| private static final InternalMongoClientSettings DEFAULT_INTERNAL_SETTINGS = InternalMongoClientSettings.getDefaults(); |
There was a problem hiding this comment.
See comment regarding this in the reactive streams version.
| final MongoClientSettings mongoClientSettings) { | ||
| return MongoClients.create(mongoClientSettings, mongoDriverInformation); | ||
| return InternalMongoClients.create(mongoClientSettings, mongoDriverInformation, | ||
| InternalMongoClientSettings.builder().recordEverything(true).build()); |
There was a problem hiding this comment.
Should this be done via a test flag: -Dorg.mongo.test.recordEverything?
Probably not - could be an option in the future if needed.
recordEverythingwith instance-basedInternalMongoClientSettingsconfiguration.InternalConnectionPoolSettingsunderInternalMongoClientSettingsFixes JAVA-6060