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

Refactor jmxserver detection in native executable build #46699

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zakkak
Copy link
Contributor

@zakkak zakkak commented Mar 10, 2025

Avoids warnings without introducing a new configuration option

Closes #46692
Reverts/Supersedes #46592
Fixes #46506

@zakkak
Copy link
Contributor Author

zakkak commented Mar 10, 2025

@roberttoyonaga can you please review this?

@zakkak
Copy link
Contributor Author

zakkak commented Mar 10, 2025

@roberttoyonaga can you please review this?

Note that there is an ongoing discussion in zulip to see if we can avoid setting the monitoring property as well https://quarkusio.zulipchat.com/#narrow/channel/187038-dev/topic/System.20property.20set.20at.20build.20time.20not.20visible.20at.20runtime

@roberttoyonaga
Copy link
Contributor

@zakkak This looks good to me and works on my machine.

@roberttoyonaga
Copy link
Contributor

roberttoyonaga commented Mar 10, 2025

@zakkak I tried what you mentioned on Zulip and was able to successfully obtain the value with:

Config config = ConfigProvider.getConfig();
String monitoringProperty = config.getConfigValue("quarkus.native.monitoring").getValue();

ConfigValue{name='quarkus.native.monitoring', value='jmxserver,jmxclient', rawValue='jmxserver,jmxclient', profile='null', configSourceName='SysPropConfigSource', configSourceOrdinal=400, configSourcePosition=0, lineNumber=-1}
That wasn't with your reproducer though, just my own test app. I'll check with your app now

@roberttoyonaga
Copy link
Contributor

roberttoyonaga commented Mar 10, 2025

It seems to work with ./mvnw clean package -Dnative -Dnative.surefire.skip -Dquarkus.native.container-runtime=podman -pl integration-tests/picocli-native -Dquarkus.native.monitoring=heapdump,jmxserver as well.

ConfigValue{name='quarkus.native.monitoring', value='heapdump,jmxserver', rawValue='heapdump,jmxserver', profile='null', configSourceName='SysPropConfigSource', configSourceOrdinal=400, configSourcePosition=0, lineNumber=-1}

@zakkak
Copy link
Contributor Author

zakkak commented Mar 10, 2025

@roberttoyonaga did you place the code snippet in io.quarkus.runtime.graal.Target_javax_management_JMX.JmxServerNotIncluded#getAsBoolean or elsewhere?

@roberttoyonaga
Copy link
Contributor

roberttoyonaga commented Mar 10, 2025

@zakkak I modified this PR branch with this code:

        @Override
        public boolean getAsBoolean() {
            Config config = ConfigProvider.getConfig();
            String monitoringProperty = config.getConfigValue("quarkus.native.monitoring").getValue();
            if (monitoringProperty != null) {
                System.out.println(config.getConfigValue("quarkus.native.monitoring"));
                return !monitoringProperty.contains("jmxserver");
            }
            return true;
        }

inside io.quarkus.runtime.graal.Target_javax_management_JMX.JmxServerNotIncluded#getAsBoolean

@zakkak
Copy link
Contributor Author

zakkak commented Mar 10, 2025

I see, I guess that works because of https://github.com/quarkusio/quarkus/pull/46699/files#diff-65211cbc596a40d560ae4669e85aefdf2fec85c0a9fc11d6c92532d7492e4303R87-R92 , but my goal is to avoid it.

Copy link

quarkus-bot bot commented Mar 10, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 7d44f25.

✅ 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

📦 extensions/panache/hibernate-reactive-rest-data-panache/deployment

io.quarkus.hibernate.reactive.rest.data.panache.deployment.entity.PanacheEntityResourcePutMethodTest.shouldUpdateSimpleObject - History

  • 1 expectation failed. JSON path name doesn't match. Expected: is "first-test" Actual: first - java.lang.AssertionError
java.lang.AssertionError: 
1 expectation failed.
JSON path name doesn't match.
Expected: is "first-test"
  Actual: first

	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:500)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:481)

⚙️ JVM Tests - JDK 17 Windows

📦 extensions/hibernate-orm/deployment

io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessInheritanceTest.testFieldAccess - History

  • Expecting actual not to be null - java.lang.AssertionError
java.lang.AssertionError: 

Expecting actual not to be null
	at io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessInheritanceTest$FieldAccessEnhancedDelegate$2.assertValue(PublicFieldAccessInheritanceTest.java:152)
	at io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessInheritanceTest.doTestFieldAccess(PublicFieldAccessInheritanceTest.java:100)
	at io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessInheritanceTest.testFieldAccess(PublicFieldAccessInheritanceTest.java:61)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:521)

@gsmet
Copy link
Member

gsmet commented Mar 11, 2025

@zakkak what's the status of this one?

@zakkak zakkak added this to the 3.21 - main milestone Mar 11, 2025
@zakkak
Copy link
Contributor Author

zakkak commented Mar 11, 2025

@gsmet I am still waiting for @radcortez's input in https://quarkusio.zulipchat.com/#narrow/channel/187038-dev/topic/System.20property.20set.20at.20build.20time.20not.20visible.20at.20runtime to see if we can avoid explicitly passing the property down to the native-image invocation

If there is time pressure to get this backported to 3.20 in time feel free to merge (as the PR IMO is fine as is, I was just hoping to be able to take it a step further). I have included the revert and the new patch so that you can just backport the later.

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