Add Client Metadata Update Support. #1708
Conversation
JAVA-5854
JAVA-5854
| dependencies { | ||
| testImplementation(libs.assertj) | ||
| } |
There was a problem hiding this comment.
AssertJ has been used in this PR and added to test-base as it is a useful library that could be shared across all modules.
JAVA-5854
vbabanin
requested review from
a team and
nhachicha
and removed request for
a team
JAVA-5854
JAVA-5854
JAVA-5854
JAVA-5854
JAVA-5870
JAVA-5870
JAVA-5870
JAVA-5870
JAVA-5870
JAVA-5870
JAVA-5870
| * @param mongoDriverInformation the driver information to append to the existing metadata | ||
| * @since 5.6 | ||
| */ | ||
| void updateMetadata(MongoDriverInformation mongoDriverInformation); |
There was a problem hiding this comment.
What's the argument for putting the method on MongoClient rather than MongoCluster?
There was a problem hiding this comment.
Just conceptually, MongoCluster acts as a view with overridden configurations (e.g., codec registry, read preference) and doesn’t own the client’s lifecycle or global state. In my opinion, placing updateMetadata on MongoCluster raises the question: does it update metadata for just that instance or for all instances? That behavior would require clear documentation.
MongoClient already handles driver-wide operations (e.g., close), so updateMetadata aligns with its role.
| * <p>This class is not part of the public API and may be removed or changed at any time</p> | ||
| */ | ||
| public class ClientMetadata { | ||
| private volatile BsonDocument clientMetadataBsonDocument; |
There was a problem hiding this comment.
I don't thing that a volatile fields is enough. If append is called concurrently by multiple threads, then one of the updates to metadata will likely be lost.
There was a problem hiding this comment.
You're right - volatile alone isn't enough for concurrent updates. I initially didn’t add locking because I expected concurrent metadata updates to be extremely rare in practice, but I’ve now added a lock to ensure consistency. Let me know what you think.
UPD: discussed offline. Removed clone() from updateMetadata(...) for clarify: 8ade58b
JAVA-5870
JAVA-5870
| * Returns mutable BsonDocument that represents the client metadata. | ||
| */ | ||
| public BsonDocument getBsonDocument() { | ||
| return clientMetadataBsonDocument; |
There was a problem hiding this comment.
Return a defensive copy (instead of a reference to this mutable instance?)
| return clientMetadataBsonDocument; | |
| return clientMetadataBsonDocument.clone(); |
| * | ||
| * Platform is appended separately to the existing platform if it does not exceed {@value MAXIMUM_CLIENT_METADATA_ENCODED_SIZE} bytes. | ||
| */ | ||
| public static BsonDocument updateClientMedataDocument(final BsonDocument clientMetadataDocument, |
There was a problem hiding this comment.
typo
| public static BsonDocument updateClientMedataDocument(final BsonDocument clientMetadataDocument, | |
| public static BsonDocument updateClientMetadataDocument(final BsonDocument clientMetadataDocument, |
| */ | ||
| public static BsonDocument updateClientMedataDocument(final BsonDocument clientMetadataDocument, | ||
| final MongoDriverInformation mongoDriverInformation) { | ||
| BsonDocument driverInformation = clientMetadataDocument.getDocument("driver"); |
There was a problem hiding this comment.
what if the user doesn't supply the driverName when usingMongoDriverInformation.builder(). Should we handle the NPE?
|
|
||
| tryWithLimit(clientMetadataDocument, d -> { | ||
| putAtPath(d, "driver.name", listToString(updatedDriverNames)); | ||
| putAtPath(d, "driver.version", listToString(updatedDriverVersions)); |
There was a problem hiding this comment.
Should we try in a dedicated tryWithLimit?
| updateDriverPlatforms.addAll(driverPlatformsToAppend); | ||
|
|
||
| tryWithLimit(clientMetadataDocument, d -> { | ||
| putAtPath(d, "driver.name", listToString(updatedDriverNames)); |
There was a problem hiding this comment.
tryWithLimit invokes the Consumer<BsonDocument> lambda twice, we could build the appended string outside the tryWithLimit
| updateDriverPlatforms.add(clientMetadataDocument.getString("platform").getValue()); | ||
| updateDriverPlatforms.addAll(driverPlatformsToAppend); | ||
|
|
||
| tryWithLimit(clientMetadataDocument, d -> { |
There was a problem hiding this comment.
Should tryWithLimit return a boolean indicating whether the document was added (i.e enough space) or not. The boolean will avoid subsequent call to tryWithLimit if it previously failed due to reached capacity
Description
This PR updates the Java sync and reactive-streams driver to support dynamic client metadata updates, enabling wrapping libraries and integrations to append metadata about themselves when they do not instantiate the
MongoClientdirectly.Pull request status:
This pull request is kept in draft until the associated specification (mongodb/specifications#1798) is approved and merged.
JAVA-5870