[Narayana] Make all Record class and BeanPopulator to init at runtime #40310

zhfeng · 2024-04-26T13:38:42Z

revert #40250

cc @mmusgrov

quarkus-bot · 2024-04-26T13:39:01Z

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

title should preferably start with an uppercase character (if it makes sense!)

_{This message is automatically generated by a bot.}

zhfeng · 2024-04-26T13:47:02Z

@gastaldi it needs to backport to 3.8

mmusgrov

I agree that the properties and classes need to be initialised at runtime. Thanks for the PR.

mmusgrov · 2024-04-26T15:13:06Z

...na-jta/deployment/src/main/java/io/quarkus/narayana/jta/deployment/NarayanaJtaProcessor.java

@@ -156,6 +170,11 @@ public void setProperties(NarayanaJtaRecorder recorder) {
            defaultProperties.remove(i);
        }
        recorder.setDefaultProperties(defaultProperties);
+        // This must be done before setNodeName as the code in setNodeName will create a TSM based on the value of this property


Remark: this code hasn't actually been added to the PR, it was already present and it's a quirk of github that it's showing up as added code.

zhfeng · 2024-04-26T16:13:58Z

The hibernate-orm-panache is failing in native build and I will investigate.

zhfeng · 2024-04-26T23:26:01Z

@mmusgrov these three classes CAN NOT be added to initlize at runtime currently.

       runtimeInit.produce(new RuntimeInitializedClassBuildItem(UserTransactionImple.class.getName()));
       runtimeInit.produce(new RuntimeInitializedClassBuildItem(TransactionManagerImple.class.getName()));
       runtimeInit.produce(new RuntimeInitializedClassBuildItem(BaseTransaction.class.getName()));

It looks like hibernate-orm-panache has something to use TransactionManager at build time. I don't have much time to do further investigation right now. So maybe we can open a followup issue?

zhfeng · 2024-04-27T01:03:40Z

@gastaldi I think the reflection increase is expected.

gsmet · 2024-04-29T08:59:55Z

@zhfeng that's a lot of increase for a lot of applications so we need a certainty here. I'm not entirely sure why we end up increasing this so much.

Maybe @zakkak can help us understand what's going on.

BTW, I would rather have the revert in a separate PR so that we revert right away. I don't want us to take the risk of having this patch released somehow.

zhfeng · 2024-04-29T09:15:31Z

Thanks @gsmet - I seperate revert commit in

Revert "Move setDefaultProperties in the static init" #40334

quarkus-bot · 2024-04-29T10:42:20Z

Status for workflow `Quarkus CI`

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

Failing Jobs

Status	Name	Step	Failures	Logs	Raw logs	Build scan
✖	Native Tests - Data5	`Build`	Failures	Logs	Raw logs	🔍
✖	Native Tests - Main	`Build`	Failures	Logs	Raw logs	🔍

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

Failures

⚙️ Native Tests - Data5 #

- Failing: integration-tests/jpa-postgresql integration-tests/jpa-postgresql-withxml

📦 integration-tests/jpa-postgresql

✖ io.quarkus.it.jpa.postgresql.ImageMetricsITCase.verifyImageMetrics line 16 - History - More details - Source on GitHub

org.opentest4j.MultipleFailuresError: 
Multiple Failures (2 failures)
	org.opentest4j.AssertionFailedError: Expected analysis_results.methods.reflection to be within range [4367 +- 3%] but was 4663 ==> expected: <true> but was: <false>
	org.opentest4j.AssertionFailedError: Expected analysis_results.fields.reflection to be within range [201 +- 3%] but was 338 ==> expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertAll.assertAll(AssertAll.java:80)
	at org.junit.jupiter.api.AssertAll.assertAll(AssertAll.java:58)
	at org.junit.jupiter.api.Assertions.assertAll(Assertions.java:3012)
	at io.quarkus.test.junit.nativeimage.NativeBuildOutputExtension.verifyImageMetrics(NativeBuildOutputExtension.java:59)

📦 integration-tests/jpa-postgresql-withxml

✖ io.quarkus.it.jpa.postgresql.ImageMetricsITCase.verifyImageMetrics line 15 - History - More details - Source on GitHub

org.opentest4j.MultipleFailuresError: 
Multiple Failures (2 failures)
	org.opentest4j.AssertionFailedError: Expected analysis_results.methods.reflection to be within range [4551 +- 3%] but was 4845 ==> expected: <true> but was: <false>
	org.opentest4j.AssertionFailedError: Expected analysis_results.fields.reflection to be within range [170 +- 3%] but was 307 ==> expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertAll.assertAll(AssertAll.java:80)
	at org.junit.jupiter.api.AssertAll.assertAll(AssertAll.java:58)
	at org.junit.jupiter.api.Assertions.assertAll(Assertions.java:3012)
	at io.quarkus.test.junit.nativeimage.NativeBuildOutputExtension.verifyImageMetrics(NativeBuildOutputExtension.java:59)

⚙️ Native Tests - Main #

- Failing: integration-tests/main

📦 integration-tests/main

✖ io.quarkus.it.main.ImageMetricsITCase.verifyImageMetrics line 15 - History - More details - Source on GitHub

org.opentest4j.MultipleFailuresError: 
Multiple Failures (1 failure)
	org.opentest4j.AssertionFailedError: Expected analysis_results.fields.reflection to be within range [496 +- 3%] but was 633 ==> expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertAll.assertAll(AssertAll.java:80)
	at org.junit.jupiter.api.AssertAll.assertAll(AssertAll.java:58)
	at org.junit.jupiter.api.Assertions.assertAll(Assertions.java:3012)
	at io.quarkus.test.junit.nativeimage.NativeBuildOutputExtension.verifyImageMetrics(NativeBuildOutputExtension.java:59)
	at io.quarkus.test.junit.nativeimage.NativeBuildOutputExtension.verifyImageMetrics(NativeBuildOutputExtension.java:46)

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17 Windows

📦 integration-tests/grpc-hibernate

✖ com.example.grpc.hibernate.BlockingRawTest.shouldAdd - History

Condition with Lambda expression in com.example.grpc.hibernate.BlockingRawTestBase was not fulfilled within 30 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in com.example.grpc.hibernate.BlockingRawTestBase was not fulfilled within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at com.example.grpc.hibernate.BlockingRawTestBase.shouldAdd(BlockingRawTestBase.java:59)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)

gsmet · 2024-04-30T11:38:53Z

We will discuss the merit of this PR later. For now we plan to fix the immediate issue with #40365

gastaldi · 2024-05-09T12:39:45Z

Is this PR still necessary?

mmusgrov · 2024-05-10T09:07:28Z

Is this PR still necessary?

I don't think so but we need @zhfeng to decide.

zhfeng · 2024-05-10T09:31:30Z

For fixing #39283 , it is not needed. But if we still want to support system properties in the native mode, it is necessary.

zakkak · 2024-05-14T12:36:01Z

Maybe @zakkak can help us understand what's going on.

I don't see anything special going on. Registering more classes for reflection is expected to increase these metrics.

RecoveryEnvironmentBean has 36 methods
CoordinatorEnvironmentBean has 62 methods
ObjectStoreEnvironmentBean has 56 methods

With a quick count over the rest of the classes as well I get a total of 192 methods being added for reflection (which were not being added before).

Assuming that even before this PR the registered methods were close to the threshold if I subtract 192 from the reported numbers I see that the result is below the 3% threshold.

quarkus-bot bot added the area/narayana Transactions / Narayana label Apr 26, 2024

This was referenced Apr 26, 2024

Move setDefaultProperties in the static init #40250

Merged

SQLException: Unable to enlist connection to existing transaction when accessing multiple persistence units in the same transaction since 3.8.2 #39283

Closed

gastaldi added triage/backport-3.8 triage/backport labels Apr 26, 2024

zhfeng force-pushed the runtime_properties branch from d999ea3 to 27b80df Compare April 26, 2024 14:31

mmusgrov approved these changes Apr 26, 2024

View reviewed changes

gastaldi approved these changes Apr 26, 2024

View reviewed changes

gastaldi added triage/waiting-for-ci Ready to merge when CI successfully finishes and removed triage/backport-3.9 labels Apr 26, 2024

This comment has been minimized.

Sign in to view

zhfeng force-pushed the runtime_properties branch from 27b80df to 4b14a90 Compare April 26, 2024 23:19

This comment has been minimized.

Sign in to view

Make all Record class and BeanPopulator to init at runtime

de2dbb2

zhfeng force-pushed the runtime_properties branch from 4b14a90 to de2dbb2 Compare April 29, 2024 09:11

quarkus-bot bot added the triage/flaky-test label Apr 29, 2024

gsmet marked this pull request as draft April 30, 2024 11:38

gastaldi removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 9, 2024

quarkusbot added the triage/backport label May 15, 2024

gsmet removed the triage/backport-3.10 label Jun 4, 2024

quarkusbot added the triage/backport label Jun 12, 2024

gsmet removed the triage/backport-3.11 label Jun 26, 2024

quarkusbot added the triage/backport label Jul 17, 2024

gsmet removed the triage/backport-3.12 label Jul 22, 2024

quarkusbot added the triage/backport label Aug 14, 2024

gsmet removed triage/backport-3.14 labels Nov 15, 2024

gsmet removed the triage/backport-3.16 label Nov 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Narayana] Make all Record class and BeanPopulator to init at runtime #40310

[Narayana] Make all Record class and BeanPopulator to init at runtime #40310

zhfeng commented Apr 26, 2024

quarkus-bot bot commented Apr 26, 2024

zhfeng commented Apr 26, 2024

mmusgrov left a comment

mmusgrov Apr 26, 2024 •

edited

Loading

zhfeng commented Apr 26, 2024

This comment has been minimized.

zhfeng commented Apr 26, 2024 •

edited

Loading

This comment has been minimized.

zhfeng commented Apr 27, 2024

gsmet commented Apr 29, 2024

zhfeng commented Apr 29, 2024

quarkus-bot bot commented Apr 29, 2024 •

edited by github-actions bot

Loading

gsmet commented Apr 30, 2024

gastaldi commented May 9, 2024

mmusgrov commented May 10, 2024

zhfeng commented May 10, 2024

zakkak commented May 14, 2024

[Narayana] Make all Record class and BeanPopulator to init at runtime #40310

Are you sure you want to change the base?

[Narayana] Make all Record class and BeanPopulator to init at runtime #40310

Conversation

zhfeng commented Apr 26, 2024

quarkus-bot bot commented Apr 26, 2024

zhfeng commented Apr 26, 2024

mmusgrov left a comment

Choose a reason for hiding this comment

mmusgrov Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

zhfeng commented Apr 26, 2024

This comment has been minimized.

zhfeng commented Apr 26, 2024 • edited Loading

This comment has been minimized.

zhfeng commented Apr 27, 2024

gsmet commented Apr 29, 2024

zhfeng commented Apr 29, 2024

quarkus-bot bot commented Apr 29, 2024 • edited by github-actions bot Loading

Status for workflow Quarkus CI

Failing Jobs

Failures

⚙️ Native Tests - Data5 #

📦 integration-tests/jpa-postgresql

📦 integration-tests/jpa-postgresql-withxml

⚙️ Native Tests - Main #

📦 integration-tests/main

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17 Windows

📦 integration-tests/grpc-hibernate

gsmet commented Apr 30, 2024

gastaldi commented May 9, 2024

mmusgrov commented May 10, 2024

zhfeng commented May 10, 2024

zakkak commented May 14, 2024

mmusgrov Apr 26, 2024 •

edited

Loading

zhfeng commented Apr 26, 2024 •

edited

Loading

quarkus-bot bot commented Apr 29, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`