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

chore(http): remove Apache HttpClient5, replace with RESTEasy client #383

Merged
merged 3 commits into from
Apr 24, 2024

Conversation

andrewazores
Copy link
Member

Welcome to Cryostat3! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed all commits using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main


Fixes: #382
Depends on #342
Depends on #368

Description of the change:

  • removes Apache HttpClient5 dependency
  • replaces HttpClient5 utilities with utilities from other dependencies (HTTP content type and response status code constants)
  • replaces HttpClient5 actual client instance for sidecar report generation with RESTEasy client, which is The Quarkus Way and already a dependency

Motivation for the change:

One less dependency to manage, more idiomatic.

How to manually test:

  1. Build PR (currently also needs manual merge of fix(reports): report generation does not cause recursive update #368)
  2. ./smoketest.bash -OGr -s localstack
  3. In another terminal, podman logs -f compose_reports_1
  4. Open web client UI, generate a report
  5. Check reports container log, verify that it received and processed the request (and therefore that the new HTTP client was used to send it the data)

@andrewazores andrewazores requested a review from a team as a code owner April 18, 2024 19:26
@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Apr 18, 2024
@andrewazores andrewazores added chore Refactor, rename, cleanup, etc. safe-to-test and removed needs-triage Needs thorough attention from code reviewers labels Apr 18, 2024
Copy link

@andrewazores
Copy link
Member Author

/build_test

Copy link

Workflow started at 4/24/2024, 1:42:06 PM. View Actions Run.

Copy link

No GraphQL schema changes detected.

Copy link

No OpenAPI schema changes detected.

@mwangggg
Copy link
Member

hmm when I run ./mvnw clean packge, the some tests are failing and it's not consistent- mainly itest.ReportGenerationTest

Copy link

CI build and push: At least one test failed ❌ (JDK17)
https://github.com/cryostatio/cryostat3/actions/runs/8820915017

@andrewazores
Copy link
Member Author

Seems like a weird failure, but I suppose it could have to do with the Future/Uni changes in the reports service implementation? I'll start there for troubleshooting.

Otherwise it could be a red herring and actually a problem that started with #396.

@mwangggg
Copy link
Member

Seems like a weird failure, but I suppose it could have to do with the Future/Uni changes in the reports service implementation? I'll start there for troubleshooting.

Otherwise it could be a red herring and actually a problem that started with #396.

Suppressed: java.lang.RuntimeException: Error injecting io.cryostat.reports.ReportSidecarService io.cryostat.reports.ReportsServiceImpl.sidecar
		at io.cryostat.reports.ReportsServiceImpl_Bean.doCreate(Unknown Source)
		at io.cryostat.reports.ReportsServiceImpl_Bean.create(Unknown Source)
		at io.cryostat.reports.ReportsServiceImpl_Bean.create(Unknown Source)
		at io.quarkus.arc.impl.AbstractSharedContext.createInstanceHandle(AbstractSharedContext.java:113)
		at io.quarkus.arc.impl.AbstractSharedContext$1.get(AbstractSharedContext.java:37)
		at io.quarkus.arc.impl.AbstractSharedContext$1.get(AbstractSharedContext.java:34)
		at io.quarkus.arc.impl.LazyValue.get(LazyValue.java:26)
		at io.quarkus.arc.impl.ComputingCache.computeIfAbsent(ComputingCache.java:69)
		at io.quarkus.arc.impl.AbstractSharedContext.get(AbstractSharedContext.java:34)
		at io.quarkus.arc.impl.ClientProxies.getApplicationScopedDelegate(ClientProxies.java:21)
		at io.cryostat.reports.ReportsServiceImpl_ClientProxy.arc$delegate(Unknown Source)
		at io.cryostat.reports.ReportsServiceImpl_ClientProxy.reportFor(Unknown Source)
		at io.cryostat.reports.Reports.getActive(Reports.java:148)
		at io.cryostat.reports.Reports$quarkusrestinvoker$getActive_6fb45eeb93da6d8561a31f41648f9370eb83b563.invoke(Unknown Source)
		at org.jboss.resteasy.reactive.server.handlers.InvocationHandler.handle(InvocationHandler.java:29)
		at io.quarkus.resteasy.reactive.server.runtime.QuarkusResteasyReactiveRequestContext.invokeHandler(QuarkusResteasyReactiveRequestContext.java:141)
		at org.jboss.resteasy.reactive.common.core.AbstractResteasyReactiveContext.run(AbstractResteasyReactiveContext.java:145)
		at io.quarkus.vertx.core.runtime.VertxCoreRecorder$14.runWith(VertxCoreRecorder.java:576)
		at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2513)
		at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1538)
		at org.jboss.threads.DelegatingRunnable.run(DelegatingRunnable.java:29)
		at org.jboss.threads.ThreadLocalResettingRunnable.run(ThreadLocalResettingRunnable.java:29)
		at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
		at java.base/java.lang.Thread.run(Thread.java:840)
	Caused by: java.lang.IllegalArgumentException: Unable to determine the proper baseUrl/baseUri. Consider registering using @RegisterRestClient(baseUri="someuri"), @RegisterRestClient(configKey="orkey"), or by adding 'quarkus.rest-client.reports.url' or 'quarkus.rest-client.reports.uri' to your Quarkus configuration
		at io.quarkus.rest.client.reactive.runtime.RestClientCDIDelegateBuilder.configureBaseUrl(RestClientCDIDelegateBuilder.java:366)
		at io.quarkus.rest.client.reactive.runtime.RestClientCDIDelegateBuilder.configureBuilder(RestClientCDIDelegateBuilder.java:64)
		at io.quarkus.rest.client.reactive.runtime.RestClientCDIDelegateBuilder.build(RestClientCDIDelegateBuilder.java:59)
		at io.quarkus.rest.client.reactive.runtime.RestClientCDIDelegateBuilder.createDelegate(RestClientCDIDelegateBuilder.java:42)
		at io.quarkus.rest.client.reactive.runtime.RestClientReactiveCDIWrapperBase.delegate(RestClientReactiveCDIWrapperBase.java:76)
		at io.quarkus.rest.client.reactive.runtime.RestClientReactiveCDIWrapperBase.<init>(RestClientReactiveCDIWrapperBase.java:30)
		at io.cryostat.reports.ReportSidecarService$$CDIWrapper.<init>(Unknown Source)
		at io.cryostat.reports.ReportSidecarService$$CDIWrapper_ClientProxy.<init>(Unknown Source)
		at io.cryostat.reports.ReportSidecarService$$CDIWrapper_Bean.proxy(Unknown Source)
		at io.cryostat.reports.ReportSidecarService$$CDIWrapper_Bean.get(Unknown Source)
		at io.cryostat.reports.ReportSidecarService$$CDIWrapper_Bean.get(Unknown Source)
		... 24 more

these are the logs I see if this helps...

@andrewazores
Copy link
Member Author

Oh nice, thanks, that's very helpful. That could be a simple rebase issue actually.

@andrewazores
Copy link
Member Author

/build_test

Copy link

Workflow started at 4/24/2024, 2:49:36 PM. View Actions Run.

Copy link

No GraphQL schema changes detected.

Copy link

No OpenAPI schema changes detected.

Copy link

CI build and push: All tests pass ✅ (JDK17)
https://github.com/cryostatio/cryostat3/actions/runs/8821744400

Copy link
Member

@mwangggg mwangggg left a comment

Choose a reason for hiding this comment

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

looks good!

@andrewazores andrewazores merged commit 19d6926 into cryostatio:main Apr 24, 2024
8 checks passed
@andrewazores andrewazores deleted the remove-apache-http5 branch April 24, 2024 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Refactor, rename, cleanup, etc. safe-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task] Downgrade HttpClient from 5.x to 4.5.14
2 participants