Move setDefaultProperties in the static init #40250

zhfeng · 2024-04-24T14:22:27Z

It related to

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

gastaldi · 2024-04-24T15:05:44Z

/cc @mmusgrov

quarkus-bot · 2024-04-24T16:02:19Z

Status for workflow `Quarkus CI`

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

✅ 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/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

⚙️ JVM Tests - JDK 21

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

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

Sanne · 2024-04-24T16:57:41Z

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

@@ -137,6 +138,16 @@ public void build(NarayanaJtaRecorder recorder,
        builder.addBeanClass(TransactionalInterceptorNotSupported.class);
        additionalBeans.produce(builder.build());

+        // This must be done before setNodeName as the code in setNodeName will create a TSM based on the value of this property
+        recorder.disableTransactionStatusManager();
+        recorder.setNodeName(transactions);


Did someone check if we end up having the same node names across all instances?

I don't think Amos actually changed these lines, the diff is showing because the other lines were reordered. I believe that the only change is to move the code that sets up the defaultProperties into a @record(STATIC_INIT) build step (he added the new method public void setProperties(NarayanaJtaRecorder recorder). Is that correct @zhfeng ?

Yeah, it is. Thanks @mmusgrov !

@Sanne I don't understand here. What are you mean all intances?

By "instances" I mean instances of the application. If I start 10 copies of the same Quarkus application, it's my understanding that it's essential that they are assigned a different Node Name - or at least to offer the possibility of users to configure different node names.

By seeing the setNodeName invoked on a build-time recorder, it makes me worry that this configuration setting wouldn't be honoured at runtime: I'm suggesting to please check this as it would be a grave mistake.

Hmm, no, setNodeName is at runtime. And this commit has been reverted in

Revert "Move setDefaultProperties in the static init" #40334

Ok, thanks.

mmusgrov

This is a much more insightful fix than the ones suggested on the issue, I approve.

gastaldi · 2024-04-24T22:52:07Z

Should we backport this too?

zhfeng · 2024-04-24T22:54:15Z

@gastaldi yeah, at least for 3.8.x

gsmet · 2024-04-26T07:59:55Z

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

@@ -145,11 +156,6 @@ public void build(NarayanaJtaRecorder recorder,
            defaultProperties.remove(i);
        }
        recorder.setDefaultProperties(defaultProperties);


With this, in the case of a native executable, the properties will be set at build time. This is what you want for all of them?

Good point - @mmusgrov do know if there is any property we don't want to set at build time?

If we only expect to apply these properties at run time. It has to revert this commit.

I'm not sure but it seems restrictive, Narayana has many useful configuration options which were added to give users runtime control over how the TM operates.

OK, I open #40310 and it looks good to make BeanPopulator to init at runtime. So we don't need any change on narayana side.

Sanne · 2024-04-28T21:53:18Z

This comment of mine here is worrying me, did someone check it?

Move setDefaultProperties in the static init #40250 (comment)

gsmet · 2024-04-29T09:27:26Z

This change was reverted in #40334 .

Move setDefaultProperties in the static init

aef89d6

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

gastaldi requested a review from gsmet April 24, 2024 15:05

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

Sanne reviewed Apr 24, 2024

View reviewed changes

mmusgrov approved these changes Apr 24, 2024

View reviewed changes

gastaldi approved these changes Apr 24, 2024

View reviewed changes

gastaldi merged commit aaf9d01 into quarkusio:main Apr 24, 2024
41 checks passed

quarkus-bot bot added this to the 3.11 - main milestone Apr 24, 2024

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

zhfeng mentioned this pull request Apr 24, 2024

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

Closed

gsmet reviewed Apr 26, 2024

View reviewed changes

zhfeng mentioned this pull request Apr 26, 2024

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

Closed

gsmet removed triage/backport-3.8 triage/backport labels Apr 29, 2024

gsmet removed this from the 3.11 - main milestone Apr 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move setDefaultProperties in the static init #40250

Move setDefaultProperties in the static init #40250

zhfeng commented Apr 24, 2024

gastaldi commented Apr 24, 2024

quarkus-bot bot commented Apr 24, 2024 •

edited by github-actions bot

Loading

Sanne Apr 24, 2024 •

edited

Loading

mmusgrov Apr 24, 2024

zhfeng Apr 24, 2024

zhfeng Apr 29, 2024

Sanne Apr 29, 2024 •

edited

Loading

zhfeng Apr 29, 2024 •

edited

Loading

Sanne Apr 29, 2024

mmusgrov left a comment

gastaldi commented Apr 24, 2024

zhfeng commented Apr 24, 2024

gsmet Apr 26, 2024

zhfeng Apr 26, 2024

zhfeng Apr 26, 2024

mmusgrov Apr 26, 2024

zhfeng Apr 26, 2024

Sanne commented Apr 28, 2024

gsmet commented Apr 29, 2024

Move setDefaultProperties in the static init #40250

Move setDefaultProperties in the static init #40250

Conversation

zhfeng commented Apr 24, 2024

gastaldi commented Apr 24, 2024

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

Status for workflow Quarkus CI

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

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

⚙️ JVM Tests - JDK 21

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

Sanne Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sanne Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

zhfeng Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmusgrov left a comment

Choose a reason for hiding this comment

gastaldi commented Apr 24, 2024

zhfeng commented Apr 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sanne commented Apr 28, 2024

gsmet commented Apr 29, 2024

quarkus-bot bot commented Apr 24, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`

Sanne Apr 24, 2024 •

edited

Loading

Sanne Apr 29, 2024 •

edited

Loading

zhfeng Apr 29, 2024 •

edited

Loading