Skip to content
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

Closed
wants to merge 4 commits into from
Closed

feat(connect): upgrade to apache httpclient 5.x #4408

wants to merge 4 commits into from

Conversation

strangelookingnerd
Copy link
Contributor

@strangelookingnerd strangelookingnerd commented Jun 6, 2024

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:

  • HTTP and SOAP connectors based on httpclient 5.x
  • Updated README.md to reflect changes
  • Trivial code cleanup

Some details I want to point out:

  • Compatibility in usage of httpclient 5 over 4 should be fine thanks to the abstraction. The only thing that really changed are the configuration options - some got removed, some got renamed, some new were added.
  • I had to remove some tests due to the config changes (but added new ones as well). Further I had to adapt some tests because they did not test properly to begin with.

I'd be happy to discuss any recommendations or changes.

- implement connectors based on httpclient 5.x
- update README.md to reflect changes
- trivial code cleanup
Copy link
Member

@tasso94 tasso94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @strangelookingnerd,

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

connect/pom.xml Outdated Show resolved Hide resolved
* preserve compatibility

Co-authored-by: tasso94 <[email protected]>
@tasso94
Copy link
Member

tasso94 commented Jun 17, 2024

Hi @strangelookingnerd,

For some reason, the org.camunda.bpm.spring.boot.starter.webapp.WebappTest#testAdminEndpointAvailable test fails for me with your changes. Here is the CI output:

Error
expected: 200 OK
 but was: 302 FOUND
Stacktrace
org.opentest4j.AssertionFailedError: 
expected: 200 OK
 but was: 302 FOUND
	at org.camunda.bpm.spring.boot.starter.webapp.WebappTest.testAdminEndpointAvailable(WebappTest.java:56)
Standard Output
____                                 _         ____  _       _    __                      
/ ___| __ _ _ __ ___  _   _ _ __   __| | __ _  |  _ \| | __ _| |_ / _| ___  _ __ _ __ ___  
| |   / _` | '_ ` _ \| | | | '_ \ / _` |/ _` | | |_) | |/ _` | __| |_ / _ \| '__| '_ ` _ \ 
| |__| (_| | | | | | | |_| | | | | (_| | (_| | |  __/| | (_| | |_|  _| (_) | |  | | | | | |
\____/\__,_|_| |_| |_|\__,_|_| |_|\__,_|\__,_| |_|   |_|\__,_|\__|_|  \___/|_|  |_| |_| |_|
  Spring-Boot:  (v3.3.0)
  Camunda Platform: (v7.22.0-SNAPSHOT)
  Camunda Platform Spring Boot Starter: (v7.22.0-SNAPSHOT)
2024-06-14T16:14:57.443+02:00  INFO 17012 --- [           main] o.c.b.s.boot.starter.webapp.WebappTest   : Starting WebappTest using Java 17 with PID 17012 (started by camunda in /home/work/workspace/22_cambpm-ce_cambpm-main_PR-4439/spring-boot-starter/starter-webapp)
2024-06-14T16:14:57.446+02:00  INFO 17012 --- [           main] o.c.b.s.boot.starter.webapp.WebappTest   : No active profile set, falling back to 1 default profile: "default"
2024-06-14T16:14:58.630+02:00  INFO 17012 --- [           main] o.s.b.w.embedded.tomcat.TomcatWebServer  : Tomcat initialized with port 0 (http)
2024-06-14T16:14:58.642+02:00  INFO 17012 --- [           main] o.apache.catalina.core.StandardService   : Starting service [Tomcat]
2024-06-14T16:14:58.643+02:00  INFO 17012 --- [           main] o.apache.catalina.core.StandardEngine    : Starting Servlet engine: [Apache Tomcat/10.1.24]
2024-06-14T16:14:58.715+02:00  INFO 17012 --- [           main] o.a.c.c.C.[Tomcat].[localhost].[/]       : Initializing Spring embedded WebApplicationContext
2024-06-14T16:14:58.716+02:00  INFO 17012 --- [           main] w.s.c.ServletWebServerApplicationContext : Root WebApplicationContext: initialization completed in 1250 ms
2024-06-14T16:14:58.935+02:00  INFO 17012 --- [           main] o.s.j.d.e.EmbeddedDatabaseFactory        : Starting embedded database: url='jdbc:h2:mem:ace64775-585a-4e9e-80d3-ae8857ba979e;DB_CLOSE_DELAY=-1;DB_CLOSE_ON_EXIT=false', username='sa'
2024-06-14T16:14:59.160+02:00  INFO 17012 --- [           main] org.camunda.bpm.spring.boot              : STARTER-SB040 Setting up jobExecutor with corePoolSize=3, maxPoolSize:10
2024-06-14T16:14:59.267+02:00  INFO 17012 --- [           main] org.camunda.bpm.engine.cfg               : ENGINE-12003 Plugin 'CompositeProcessEnginePlugin[genericPropertiesConfiguration, camundaProcessEngineConfiguration, camundaDatasourceConfiguration, camundaJobConfiguration, camundaHistoryConfiguration, camundaMetricsConfiguration, camundaAuthorizationConfiguration, failedJobConfiguration, disableDeploymentResourcePattern, eventPublisherPlugin, ApplicationContextClassloaderSwitchPlugin]' activated on process engine 'default'
2024-06-14T16:14:59.271+02:00  INFO 17012 --- [           main] org.camunda.bpm.spring.boot              : STARTER-SB020 ProcessApplication enabled: autoDeployment via springConfiguration#deploymentResourcePattern is disabled
2024-06-14T16:14:59.272+02:00  INFO 17012 --- [           main] o.c.b.s.b.s.event.EventPublisherPlugin   : EVENTING-001: Initialized Camunda Spring Boot Eventing Engine Plugin.
2024-06-14T16:14:59.273+02:00  INFO 17012 --- [           main] o.c.b.s.b.s.event.EventPublisherPlugin   : EVENTING-003: Task events will be published as Spring Events.
2024-06-14T16:14:59.273+02:00  INFO 17012 --- [           main] o.c.b.s.b.s.event.EventPublisherPlugin   : EVENTING-005: Execution events will be published as Spring Events.
2024-06-14T16:14:59.273+02:00  INFO 17012 --- [           main] o.c.b.s.b.s.event.EventPublisherPlugin   : EVENTING-009: Listeners will not be invoked if a skipCustomListeners API parameter is set to true by user.
2024-06-14T16:14:59.279+02:00  INFO 17012 --- [           main] o.c.b.s.b.s.event.EventPublisherPlugin   : EVENTING-007: History events will be published as Spring events.
2024-06-14T16:14:59.484+02:00  INFO 17012 --- [           main] org.camunda.feel.FeelEngine              : Engine created. [value-mapper: CompositeValueMapper(List(org.camunda.feel.impl.JavaValueMapper@4537880f)), function-provider: org.camunda.bpm.dmn.feel.impl.scala.function.CustomFunctionTransformer@503556cb, clock: SystemClock, configuration: {externalFunctionsEnabled: false}]
2024-06-14T16:15:01.603+02:00  INFO 17012 --- [           main] org.camunda.bpm.connect                  : CNCT-01004 Discovered provider for connector id 'http-connector' and class 'org.camunda.connect.httpclient.impl.HttpConnectorImpl': 'org.camunda.connect.httpclient.impl.HttpConnectorProviderImpl'
2024-06-14T16:15:01.605+02:00  INFO 17012 --- [           main] org.camunda.bpm.connect                  : CNCT-01004 Discovered provider for connector id 'soap-http-connector' and class 'org.camunda.connect.httpclient.soap.impl.SoapHttpConnectorImpl': 'org.camunda.connect.httpclient.soap.impl.SoapHttpConnectorProviderImpl'
2024-06-14T16:15:01.896+02:00  INFO 17012 --- [           main] org.camunda.bpm.engine                   : ENGINE-00001 Process Engine default created.
2024-06-14T16:15:02.315+02:00  INFO 17012 --- [           main] o.c.b.s.b.s.w.f.LazyInitRegistration     : lazy initialized org.camunda.bpm.spring.boot.starter.webapp.filter.LazySecurityFilter@c412556
2024-06-14T16:15:02.316+02:00  INFO 17012 --- [           main] o.c.b.s.b.s.w.f.LazyInitRegistration     : lazy initialized org.camunda.bpm.spring.boot.starter.webapp.filter.LazyProcessEnginesFilter@5f395ce1
2024-06-14T16:15:02.478+02:00  INFO 17012 --- [           main] o.s.b.w.embedded.tomcat.TomcatWebServer  : Tomcat started on port 45167 (http) with context path '/'
2024-06-14T16:15:02.558+02:00  INFO 17012 --- [           main] org.camunda.bpm.container                : ENGINE-08026 No processes.xml file found in process application 'webappExampleApplication'
2024-06-14T16:15:02.561+02:00  INFO 17012 --- [           main] org.camunda.bpm.container                : ENGINE-08050 Process application webappExampleApplication successfully deployed
2024-06-14T16:15:02.563+02:00  INFO 17012 --- [           main] o.c.b.s.boot.starter.webapp.WebappTest   : Started WebappTest in 5.388 seconds (process running for 6.457)
2024-06-14T16:15:02.566+02:00  INFO 17012 --- [           main] org.camunda.bpm.engine.jobexecutor       : ENGINE-14014 Starting up the JobExecutor[org.camunda.bpm.engine.spring.components.jobexecutor.SpringJobExecutor].
2024-06-14T16:15:02.567+02:00  INFO 17012 --- [ingJobExecutor]] org.camunda.bpm.engine.jobexecutor       : ENGINE-14018 JobExecutor[org.camunda.bpm.engine.spring.components.jobexecutor.SpringJobExecutor] starting to acquire jobs
2024-06-14T16:15:03.398+02:00  INFO 17012 --- [o-auto-1-exec-1] o.a.c.c.C.[Tomcat].[localhost].[/]       : Initializing Spring DispatcherServlet 'dispatcherServlet'
2024-06-14T16:15:03.398+02:00  INFO 17012 --- [o-auto-1-exec-1] o.s.web.servlet.DispatcherServlet        : Initializing Servlet 'dispatcherServlet'
2024-06-14T16:15:03.400+02:00  INFO 17012 --- [o-auto-1-exec-1] o.s.web.servlet.DispatcherServlet        : Completed initialization in 1 ms
2024-06-14T16:15:03.599+02:00  INFO 17012 --- [ionShutdownHook] org.camunda.bpm.container                : ENGINE-08051 Process application webappExampleApplication undeployed
2024-06-14T16:15:03.619+02:00  INFO 17012 --- [ionShutdownHook] org.camunda.bpm.engine.jobexecutor       : ENGINE-14015 Shutting down the JobExecutor[org.camunda.bpm.engine.spring.components.jobexecutor.SpringJobExecutor]
2024-06-14T16:15:03.620+02:00  INFO 17012 --- [ingJobExecutor]] org.camunda.bpm.engine.jobexecutor       : ENGINE-14020 JobExecutor[org.camunda.bpm.engine.spring.components.jobexecutor.SpringJobExecutor] stopped job acquisition
2024-06-14T16:15:03.624+02:00  INFO 17012 --- [ionShutdownHook] org.camunda.bpm.engine                   : ENGINE-00007 Process Engine default closed

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,
Tassilo

@strangelookingnerd
Copy link
Contributor Author

Can you reproduce this locally on your machine?

I indeed could see that test fail locally as well. Since I would not know how my changes could affect that test specifically,
I just updated my branch to include latest changes from master. With that the tests run locally without any issue.

@tasso94
Copy link
Member

tasso94 commented Jun 17, 2024

Let me try to rebase my CI run PR as well. I'll let you know if the failure persists.

@tasso94
Copy link
Member

tasso94 commented Jun 18, 2024

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:

$ cd ./qa 
$ mvn clean install -Pwildfly,h2,engine-integration-jakarta

@strangelookingnerd
Copy link
Contributor Author

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:

$ cd ./qa 
$ mvn clean install -Pwildfly,h2,engine-integration-jakarta

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.

@tasso94
Copy link
Member

tasso94 commented Jun 18, 2024

No worries.

As a first step, can you consolidate the scattered version properties? I.e., update the version.httpclient property in the parent/pom.xml to version 5 and remove the version properties from clients/java/client/pom.xml and connect/pom.xm?

When you did this, I will check if the errors are gone. If not, we will investigate this further.

- consolidate version.httpclient property
@strangelookingnerd
Copy link
Contributor Author

As a first step, can you consolidate the scattered version properties?

Done in 60d0f2c

@strangelookingnerd strangelookingnerd closed this by deleting the head repository Jul 19, 2024
@Nandanrshenoy
Copy link
Contributor

Hi @strangelookingnerd,
We have some dependency on this change that you're Contributing. Do you have some updates for us on when this feature can be brought to closure :).

Thanks and Regards,
Nandan Shenoy

@strangelookingnerd
Copy link
Contributor Author

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.

@Nandanrshenoy
Copy link
Contributor

@yanavasileva,Can you kindly see if someone can take up this review.Thanks.

@yanavasileva
Copy link
Member

The PR has been closed and I can't reopen it to continue the work on it.
@strangelookingnerd, could you please try to reopen the PR. Maybe you need to raise a new one since head repository has been deleted. Also don't forget to rebase against latest master.

Once there's an open PR for the topic, someone of the team will continue with the review of the item.
Also, keep in mind that the team has other responsibilities and we do our best to provide feedback on community contributions.

@strangelookingnerd
Copy link
Contributor Author

The PR has been closed and I can't reopen it to continue the work on it. @strangelookingnerd, could you please try to reopen the PR. Maybe you need to raise a new one since head repository has been deleted. Also don't forget to rebase against latest master.

See #4408

Once there's an open PR for the topic, someone of the team will continue with the review of the item. Also, keep in mind that the team has other responsibilities and we do our best to provide feedback on community contributions.

I get that, but 2 month without feedback is not really helpful either. The initial PR was created in April (camunda/camunda-connect#89)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants