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.

Sign up for GitHub

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

Jump to bottom

Reduce our exposure to wildfly-common #45751

Merged
merged 2 commits into from
Jan 22, 2025
Merged

Reduce our exposure to wildfly-common #45751

merged 2 commits into from
Jan 22, 2025

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Jan 21, 2025

Ideally we would only rely on SmallRye Common when we can but:

  • We have JBoss Threads depending on wildfly-common anyway
  • We use Locks and Flags which don't have an equivalent in SmallRye Common

The point of this is that using both might be counter productive if for instance some static fields are initialized. Now I'm not sure exactly what was the plan but SmallRye Common contains more and more elements from wildfly-common.

Draft as this is more to start a discussion than anything else.

/cc @dmlloyd

@gsmet
Copy link
Member Author

gsmet commented Jan 21, 2025

Also /cc @radcortez

@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/config area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/hibernate-orm Hibernate ORM area/logging area/maven area/rest area/security area/spring Issues relating to the Spring integration area/testing area/vertx labels Jan 21, 2025
@dmlloyd
Copy link
Member

dmlloyd commented Jan 21, 2025

The methods in jboss-threads which use the functions from wildfly-common are all deprecated, so over time that will phase out. I'm not sure there's a good way to phase out flags usage but that API is somewhat internal so maybe the exposure is minimal.

gsmet
gsmet commented Jan 21, 2025
View reviewed changes
core/runtime/src/main/java/io/quarkus/runtime/logging/LoggingSetupRecorder.java
Comment on lines +3 to -6
import static io.smallrye.common.net.HostName.getQualifiedHostName;
import static io.smallrye.common.os.Process.getProcessName;
import static java.util.Collections.emptyList;
import static java.util.Collections.emptyMap;
import static org.wildfly.common.net.HostName.getQualifiedHostName;
import static org.wildfly.common.os.Process.getProcessName;
Copy link
Member Author

Choose a reason for hiding this comment

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

TBH, this is what made me think about it. Mostly that it could be counter productive having things depending on both implementations.

Copy link
Member

Choose a reason for hiding this comment

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

I think this makes sense. The main cost is borne by class loading, so every redundant class we don't load is a win even if we still use a couple of classes from the library in the end.

@gsmet
Copy link
Member Author

gsmet commented Jan 21, 2025

@dmlloyd yeah, in the end, I would like dropping the dependency to wilfly-common except when it's absolutely needed (for instance, when we are using WildFly APIs).

FWIW, I added a comment that explains the rationale of this PR. Most of it is noise but what I pointed out might be of interest.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 21, 2025 •
edited
Loading

🎊 PR Preview f22f823 has been successfully built and deployed to https://quarkus-pr-main-45751-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

@radcortez
Copy link
Member

Maybe we can also ban in the build the APIs from WF Common that have an equivalent in SR Common.

@gsmet
Copy link
Member Author

gsmet commented Jan 22, 2025

@radcortez we probably could ban some of them. Problem is, you have to be extremely specific as in some cases, you have to use WildFly Common API (for instance when talking to Elytron).

Also while we have forbiddenapis enabled in some modules, it's not globally enabled.

So I would be in favor of it but I wonder if it's not going to require a lot of investment. Maybe we could try to ban the Net and CPU things from Core + Vert.x + Vert.x HTTP. That would be a good first step.

Ideally I would like to drop the dependency from core dependencies (but we will have to wait for JBoss Threads to drop its dependency to it - and I'm perfectly aware that we need to be quite conservative for JBoss Threads).

gsmet added 2 commits January 22, 2025 10:18
@gsmet
Reduce our exposure to wildfly-common
b08de8b 
Ideally we would only rely on SmallRye Common when we can but:

- We have JBoss Threads depending on wildfly-common anyway
- We use Locks and Flags which don't have an equivalent in SmallRye
  Common
@gsmet
Add forbiddenapis in core runtime packages and ban some WildFly Common
513edc3
@gsmet gsmet force-pushed the get-rid-of-wildfly-common branch from 9900734 to 513edc3 Compare January 22, 2025 09:40
@gsmet
Copy link
Member Author

gsmet commented Jan 22, 2025

@radcortez I pushed an additional commit with some ForbiddenAPIs adjustments. It's still not run everywhere but it should at least improve things in the critical Core modules.

We can think a bit more about it and decide if we want to generalize it but it should be done in another PR IMHO.

@gsmet gsmet marked this pull request as ready for review January 22, 2025 09:42
@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Jan 22, 2025

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 513edc3.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

@radcortez
Copy link
Member

Sounds good!

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Jan 22, 2025 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 513edc3.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 integration-tests/observability-lgtm

✖ io.quarkus.observability.test.LgtmLifecycleTest.testTracing - History

  • java.lang.RuntimeException: java.lang.reflect.InvocationTargetException - java.lang.RuntimeException
java.lang.RuntimeException: java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
	at io.quarkus.test.junit.QuarkusTestExtension.throwBootFailureException(QuarkusTestExtension.java:611)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestClassConstructor(QuarkusTestExtension.java:695)
	at java.base/java.util.Optional.orElseGet(Optional.java:364)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
	at io.quarkus.test.junit.TestResourceUtil$TestResourceManagerReflections.startReflectively(TestResourceUtil.java:214)

@gsmet gsmet merged commit 16c3806 into quarkusio:main Jan 22, 2025
67 of 69 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.19 - main milestone Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmlloyd dmlloyd dmlloyd left review comments

@radcortez radcortez radcortez approved these changes

Assignees
No one assigned
Labels
area/arc Issue related to ARC (dependency injection) area/config area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/hibernate-orm Hibernate ORM area/logging area/maven area/rest area/security area/spring Issues relating to the Spring integration area/testing area/vertx triage/flaky-test
Projects
None yet
Milestone
3.19 - main
Development

Successfully merging this pull request may close these issues.
3 participants
@gsmet @dmlloyd @radcortez