-
Notifications
You must be signed in to change notification settings - Fork 414
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
OAK-9447: Upgrade Mongo java driver to 5.2 #1761
base: trunk
Are you sure you want to change the base?
Conversation
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.
First of all, thanks for working on that.
That said, this would be much easier to review if you wouldn't have touched inport statement order. Any chance you could take this part of the changes out?
Thanks Julian and sorry for having changed the import order, in eclipse I leave it enabled by default, I find it very useful :) I'll try to restore the import order... |
Hi I've just committed import reordering on almost all classes hoping that this will make easier the evaluation, bye |
Thanks @rgambelli, I have approved the execution of tests on our pipeline to see if tests are passing. |
@@ -373,8 +368,8 @@ public void apply(BasicDBObject document) { | |||
*/ | |||
@Override | |||
public Optional<NodeDocument> getOldestModifiedDoc(final Clock clock) { | |||
final Bson sort = and(eq(MODIFIED_IN_SECS, 1), eq(ID, 1)); | |||
|
|||
final Bson sort = Sorts.ascending(MODIFIED_IN_SECS, ID); |
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.
Could you please create a JIRA issue for this bug and submit a separate Pull Request? I'd like to separate bug fixes from other changes.
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.
Sorry @reschke could you guide me in doing what you are asking?
I've created the new issue 11169, now how can I submit new pull request while there is already this open? Should I create a new branch? In current pull-request should I rollback the fix in MongoVersionGCSupport leaving the code as I found initially?
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 would recommend that while the biggger change gets more testing, we treat the query bug as seperate issue and fix it, "just because". Once the fix is in, we can rebase the PR (or merge), and can proceed with this one.
I have created https://issues.apache.org/jira/browse/OAK-11170 to first address the deprecations (and assigned it to me). That should make this PR a bit smaller. |
@@ -2345,11 +2339,6 @@ private boolean withClientSession() { | |||
return connection.getStatus().isClientSessionSupported() && useClientSession; | |||
} | |||
|
|||
private boolean secondariesWithinAcceptableLag() { |
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.
Is it ok to drop this code?
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.
In my opinion yes, I wrote you an email on 2 october providing my opinion about that matter, I copy here what I wrote in that email:
I would like to share with you that the code in MongoDocumentStore like this:
if (secondariesWithinAcceptableLag()) {
dbCollection = getDBCollection(collection);
} else {
lagTooHigh();
dbCollection = getDBCollection(collection).withReadPreference(ReadPreference.primary());
}
will be replaced simply with this:
dbCollection = getDBCollection(collection);
because mongodb-driver-sync knows itself if ask to primary or secondary based on maxStalenessSeconds property
ReplicaSetStatus class too has not much more sense I think, for sure its method updateLag
Have you some opinion you want to share?
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.
Based on the documentation provided, it seems ok to remove this code, as the driver now does this check internally.
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.
Have you some opinion you want to share?
Nope, I'm not a MongoDB expert, it just seemed to be suspicious.
Hi @rgambelli, thanks for your contribution. Could you please update the supported driver versions in the I have an example I did in a previous PR: https://github.com/apache/jackrabbit-oak/pull/295/files#diff-a5e72d35ad0d76d993dd91eec5b7cd18ca596653345c0bd37c088ffeda4a0031R47-R48 With this change I don't think 3.8+ is supported anymore, so probably the supported versions should go from [5.0, 5.2]. Assuming there are no breaking changes from 5.0 to 5.2. |
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/MongoUtils.java
Outdated
Show resolved
Hide resolved
@reschke about last conflict I'm not sure to understand if it's something I have to do myself or you will do it? |
@rgambelli - I'll now move the PR over to Apache git, rebase it, test it once more then merge it. |
https://issues.apache.org/jira/browse/OAK-9447
I've just finished with the replacement of java-mongo-driver with the latest mongodb-driver-sync 5.2.0 published on Sep 24, 2024.
Main changes are in oak-document-store project, mongodb-driver-sync has no CVE at all, that driver is also a dependency of the spring ecosystem, this means have no errors if you use oak inside a springboot application, spring actuator expects classes from mongodb-driver-sync which can't find in mongo-java-driver.
It should be more performant too, this is stated in web, I haven't done performance tests.