-
Notifications
You must be signed in to change notification settings - Fork 473
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
JAMES-3929 Using http5 client instead of opensearch rest client #1648
base: master
Are you sure you want to change the base?
Conversation
server/testing/src/main/java/org/apache/james/util/docker/Images.java
Outdated
Show resolved
Hide resolved
Seems to be the same issue that @vttranlina encountered... Weird cause when I tested locally quickly yesterday it seemed ok. I will dig more today |
...-common/opensearch/src/main/java/org/apache/james/backends/opensearch/OpenSearchIndexer.java
Outdated
Show resolved
Hide resolved
Hmmmm seems hanging forever same in some other tests in |
36d5d46
to
5eaf97a
Compare
Likely related, and a nice problem to debug! At your flame graphs / visualvm dumps! |
I guessed with what I saw with perf tests yesterday... I had 9000+ users (10k users total) hanging at the end of the imap simulation... I wanted to do some dumps and graphs today but well... can't access the perf test VM for now |
not related but don't forget the MAX_DURATION env variable |
I didn't and the simulation finished because of it. But when before finishing you see more than 9k users are still hanging, you know something is wrong :) |
5eaf97a
to
2a08702
Compare
What is the status of this work? |
Couldn't find a way to solve it |
backends-common/opensearch/pom.xml
Outdated
<!-- Needed for opensearch-java dependency --> | ||
<dependency> | ||
<groupId>org.apache.httpcomponents.client5</groupId> | ||
<artifactId>httpclient5</artifactId> | ||
<version>5.1.4</version> | ||
</dependency> | ||
<!-- Needed for opensearch-java dependency --> | ||
<dependency> | ||
<groupId>org.apache.httpcomponents.core5</groupId> | ||
<artifactId>httpcore5</artifactId> | ||
<version>5.1.5</version> | ||
</dependency> |
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.
Versions are not the same. Is this intendeed?
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.
Yes. Just took what was on the java opensearch client side. If you look at the respective versions at maven central as well, you can see those two libs are not strictly coupled:
- https://mvnrepository.com/artifact/org.apache.httpcomponents.client5/httpclient5
- https://mvnrepository.com/artifact/org.apache.httpcomponents.core5/httpcore5
You can see there is no 5.1.5 version for httpclient5, it stops at 1.5.4 (similar with the current 5.2.x upper version actually)
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.
httpcore5 is brought by httpclient5 cf https://mvnrepository.com/artifact/org.apache.httpcomponents.client5/httpclient5/5.1.4
IMO we do not need this dependency explicilty
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.
Please do a stricter dependency analysis with maven: mvn dependency:tree -Dverbose
in this module
(ad put the resulting output in a TXT file here)
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.
Worth a try, forgot which one I added first back then to see the compiler still crying something was missing.
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 forgot where I found that last time but I remember the scope of those deps in opensearch-java client was runtime, as you can see here:
deps.txt
Agreed that httpclient5 brings along httpcore5 though, unecessary indeed
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.
Conflicts:
[INFO] | +- org.opensearch.client:opensearch-rest-client:jar:2.7.0:runtime
[INFO] | | +- (org.apache.httpcomponents:httpclient:jar:4.5.14:runtime - version managed from 4.5.13; omitted for duplicate)
Likey an issue... The offender is restassured. Please force this dependency, no need to specify the version as we manage it in the root POM.
Idem forcethe version of httpcore to 4.4.16...
The conflict likely occurs because of restassured thus that would be why the leakwould only happen in tests.
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.
Not sure to get it..? httpclient5 and httpcore5 are different libs from httpcore and httpclient? They even have different group ids. I don't see the correlation here
Reminder that the leak blew up in my face big time during perf tests on sandbox, so not only happening in our test suite
Or did i miss something?
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 could see dependencies between them. Not as unlinked as one might think...
2a08702
to
39fa24e
Compare
Hoping that the lib update fixes the previous encountered issues. I didn't seem to encounter anymore the issue we had before on Still needs of course to be perf tested before being potentially approved/merged. |
39fa24e
to
e05d5b5
Compare
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.
Read it
Green... I guess it's time to see the perf side of things :) |
Don't forget to pick a scenario where OpenSearch is actually called... |
The full platform scenarii actually do call them... among other things. Thinking it's good enough for a first run at it. Have something more specific in mind maybe to push it further on a 2nd test run? |
e05d5b5
to
ce7c6d8
Compare
No description provided.