-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(connect): upgrade to apache httpclient 5.x #4408
feat(connect): upgrade to apache httpclient 5.x #4408
Conversation
- implement connectors based on httpclient 5.x - update README.md to reflect changes - trivial code cleanup
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.
Looks already quite good to me. I added some review hints below. ⬇️
I just started the CI to see if your changes break our tests.
May I ask you to add a paragraph to our migration guide from 7.21 to 7.22 outlining the update from Apache HTTP Client 4.0 to 5.0 and the breaking changes from a user perspective? I would mainly list the removed/changed configuration properties here.
Migration guide: https://github.com/camunda/camunda-docs-manual/blob/master/content/update/minor/721-to-722/_index.md
Best,
Tassilo
* preserve compatibility Co-authored-by: tasso94 <[email protected]>
For some reason, the
I would assume this is not due to CI flakiness since I retriggered the CI three times in a row and can observe the same problem constantly. The CI for our master branch succeeds without the issue. Can you reproduce this locally on your machine? Best, |
I indeed could see that test fail locally as well. Since I would not know how my changes could affect that test specifically, |
Let me try to rebase my CI run PR as well. I'll let you know if the failure persists. |
The failing test I mentioned earlier passes after a rebase. Now, the WildFly CI fails since it tries to use the Apache HTTP Client 4 modules. After consolidating the Apache HTTP Client version properties to one central property in the Maven pom.xml files, I think this might be mostly resolved. You can run the WildFly integration tests as follows:
|
I had a go at fixing it and I honestly think this goes way beyond the scope of the PR and more importantly beyond what I can comprehend with the given documentation of these modules. I'll need some support in fixing this. |
No worries. As a first step, can you consolidate the scattered version properties? I.e., update the When you did this, I will check if the errors are gone. If not, we will investigate this further. |
- consolidate version.httpclient property
Done in 60d0f2c |
Hi @strangelookingnerd, Thanks and Regards, |
Hey @Nandanrshenoy, I have not heard back from @tasso94 for quite some time now and #4439 has not been updated either. I have since moved on from this topic however @tasso94 can hopefully tell you more about the status. |
@yanavasileva,Can you kindly see if someone can take up this review.Thanks. |
The PR has been closed and I can't reopen it to continue the work on it. Once there's an open PR for the topic, someone of the team will continue with the review of the item. |
See #4408
I get that, but 2 month without feedback is not really helpful either. The initial PR was created in April (camunda/camunda-connect#89) |
Since #4318 got merged I re-created this PR from camunda/camunda-connect#89
According to https://docs.camunda.org/manual/7.21/update/minor/720-to-721/#update-the-client-s-apache-httpclient-to-version-5 there is already some migration towards
httpclient5
.This PR includes:
Some details I want to point out:
I'd be happy to discuss any recommendations or changes.