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

Keep static instance field and delete unused field in substitution #38117

Merged
merged 1 commit into from
Jan 10, 2024
Merged

Keep static instance field and delete unused field in substitution #38117

merged 1 commit into from
Jan 10, 2024

Conversation

zakkak
Copy link
Contributor

@zakkak zakkak commented Jan 10, 2024 •
edited
Loading

  • Don't replace com.jayway.jsonpath.internal.DefaultsImpl#INSTANCE,
    there is no need.

  • Delete com.jayway.jsonpath.internal.DefaultsImpl#mappingProvider in
    io.smallrye.reactive.kafka.graal.Target_com_jayway_jsonpath_internal_DefaultsImpl
    since it's no longer used.

Closes #37862

Being tested with Mandrel 24.0-dev in https://github.com/graalvm/mandrel/actions/runs/7472873285

@zakkak
Keep static instance field and delete unused field in substitution
36197b5 
* Don't replace `com.jayway.jsonpath.internal.DefaultsImpl#INSTANCE`,
there is no need.

* Delete `com.jayway.jsonpath.internal.DefaultsImpl#mappingProvider` in
`io.smallrye.reactive.kafka.graal.Target_com_jayway_jsonpath_internal_DefaultsImpl`
since it's no longer used.

Closes quarkusio#37862
@zakkak zakkak force-pushed the 2024-01-10-fix-37862 branch from aefb571 to 36197b5 Compare January 10, 2024 09:29
@zakkak zakkak changed the title Reset unused field in substitution Keep static instance field and delete unused field in substitution Jan 10, 2024
zakkak
zakkak commented Jan 10, 2024
View reviewed changes
...afka-client/runtime/src/main/java/io/smallrye/reactive/kafka/graal/StrimziSubstitutions.java

@RecomputeFieldValue(kind = RecomputeFieldValue.Kind.FromAlias)
@Alias
public static Target_com_jayway_jsonpath_internal_DefaultsImpl INSTANCE = new Target_com_jayway_jsonpath_internal_DefaultsImpl();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ozangunalp do you recall why you needed this field recomputation?

Copy link
Contributor

Choose a reason for hiding this comment

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

My thinking was that because DefaultsImpl holds a field value of JsonSmartMappingProvider, recalculating the static instance would be needed to remove any instance of JsonSmartMappingProvider from the native image.

Copy link
Contributor

Choose a reason for hiding this comment

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

But now that I re-run with or without I don't see any difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good, the deletion of the mappingProvider field in this PR should take care of your initial concern as well then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, Thanks!

@zakkak zakkak marked this pull request as ready for review January 10, 2024 10:38
@zakkak
Copy link
Contributor Author

zakkak commented Jan 10, 2024

@ozangunalp could you please approve the PR if you are OK with the changes? Tests results with Mandrel 24.0-dev look good, so I consider it ready for review.

@zakkak zakkak added triage/waiting-for-ci Ready to merge when CI successfully finishes area/native-image labels Jan 10, 2024
@ozangunalp
Copy link
Contributor

The test failure on JDK 21 doesn't seem related: failure of apicurio test container start

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Jan 10, 2024 •
edited by github-actions bot
Loading

Failing Jobs - Building 36197b5

Status Name Step Failures Logs Raw logs Build scan
✔️ JVM Tests - JDK 17 🔍
✖ JVM Tests - JDK 21 Build Failures Logs Raw logs 🔍

Full information is available in the Build summary check run.
You can consult the Develocity build scans.

Failures

⚙️ JVM Tests - JDK 21 #

- Failing: extensions/smallrye-reactive-messaging-kafka/deployment 
! Skipped: integration-tests/kafka-oauth-keycloak integration-tests/kafka-sasl-elytron integration-tests/opentelemetry-reactive-messaging and 5 more

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - More details - Source on GitHub

java.lang.RuntimeException: 
io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.apicurio.registry.devservice.DevServicesApicurioRegistryProcessor#startApicurioRegistryDevService threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image quay.io/apicurio/apicurio-registry-mem:2.4.2.Final
	at io.quarkus.apicurio.registry.devservice.DevServicesApicurioRegistryProcessor.startApicurioRegistryDevService(DevServicesApicurioRegistryProcessor.java:87)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:849)
	at io.quarkus.builder.BuildContext.run(BuildContext.java:256)

@zakkak zakkak merged commit 53f95bb into quarkusio:main Jan 10, 2024
37 of 38 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Jan 10, 2024
@zakkak zakkak deleted the 2024-01-10-fix-37862 branch January 10, 2024 11:26
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ozangunalp ozangunalp ozangunalp approved these changes

Assignees
No one assigned
Labels
area/kafka area/native-image kind/bugfix
Projects
None yet
Milestone
3.7.0.CR1
Development

Successfully merging this pull request may close these issues.

[GraalVM for JDK 22] kafka-oauth-keycloak native integration test fails with latest GraalVM sources
2 participants
@zakkak @ozangunalp