Introduce enableRecovery in Nayarana extension #28025

zhfeng · 2022-09-17T01:51:59Z

zhfeng · 2022-09-17T01:55:48Z

extensions/agroal/runtime/src/main/java/io/quarkus/agroal/runtime/DataSources.java

@@ -274,8 +280,9 @@ private void applyNewConfiguration(AgroalDataSourceConfigurationSupplier dataSou
            TransactionIntegration txIntegration = new NarayanaTransactionIntegration(transactionManager,
                    transactionSynchronizationRegistry, null, false,
                    dataSourceJdbcBuildTimeConfig.transactions == io.quarkus.agroal.runtime.TransactionIntegration.XA
-                            ? xaResourceRecoveryRegistry
-                            : null);
+                            && transactionRuntimeConfig.enableXARecovery


@gsmet should we log a WARN message here if quarkus.datasource.jdbc.transaction=XA but recovery is not enable on startup?

What you could do is to keep the boolean but make it an Optional one and enable recovery by default if XA is enabled.

Thus, if XA is enabled, you have the default behavior and if someone really wants to disable it, they can. By doing this, you wouldn't need a warning.

Ah, except I see enabling XA is part of Agroal and not the Narayana extension.

Wait for a few minutes, I'm going to post a proper actionable review so that we get out of this situation for 2.13 without reverting your patch.

gsmet

OK, so I thought a bit more about it.
2.13 is soon, the CR1 has already been released and I don't want us to introduce big changes regarding transaction handling post CR1.

So, let's keep it simple:

the ObjectStore directory thing was already a problem when XA was enabled. So let's not solve this issue in this PR. Keep the original config and how it worked before. The only change you introduced in your PR is that this directory is always created and it the way and we don't want that so let's solve this and we can discuss the ObjectStore thing calmly during the 2.14 cycle.
keep the new boolean you introduced for enabling XA recovery. Make it false by default. That way, it's not in the way of the rest.
rename this very boolean to enableXaRecovery as I'm unsure how the automatic config property dashification will work with the current name. Going with the name I suggest will solve all the potential problems and generate enable-xa-recovery which is what we want.
you can log a warning when XA is enabled and enableXaRecovery is false if you think it's really a problem. My guess is that we will be able to get rid of it at some point with tighter integration with Agroal. But this will have to wait for 2.14.

Does it make sense?

If anything is unclear, let's discuss it on Zulip.

zhfeng · 2022-09-19T09:30:29Z

Thanks @gsmet and it is very clear now!

extensions/agroal/runtime/src/main/java/io/quarkus/agroal/runtime/DataSources.java

gsmet · 2022-09-20T12:42:44Z

I squashed the commits and rebased. Let's wait for CI and merge.

mmusgrov · 2022-09-20T12:47:21Z

...a/runtime/src/main/java/io/quarkus/narayana/jta/runtime/TransactionManagerConfiguration.java

+     * Enable recovery service to start on startup
+     */
+    @ConfigItem(defaultValue = "false")
+    public boolean enableXaRecovery;


Do we have to restrict recovery to just JTA? If we call it enableRecovery then the same property will work for all of our transaction models (in fact, enabling recovery for just JTA would add some complexity).

BTW I argued on the other PR (#28003 (comment)) that making recovery configurable is dangerous and potentially exposes JTA users to non-ACID behaviour.

If people insist on this change, which I advise against, then you should at least call the property disableXaRecovery with a default of true and a strong caveat - that way it looks more like a workaround for a bug rather than an optional feature.

We can rename it to enableRecovery if you prefer. My understanding is that it only made sense for XA. But if not, I can rename it. Just let me know soon because the window is closing fast.

The situation is quite simple: this patch had annoying consequences for our users, so it was either revert it altogether for 2.13 and find solutions in the future or find a compromise for 2.13 and keep the patch in there (which doesn't prevent more discussions and improvements in the future). We went with the latter.

I'm renaming the property and will push soon. I think we need a discussion with you, @zhfeng , @Sanne to decide what we want to do for 2.14 with the recovery service, when it is needed, when it should be enforced by default...

gsmet · 2022-09-20T13:03:17Z

I just pushed the rename from enableXaRecovery to enableRecovery.

mmusgrov · 2022-09-20T13:10:12Z

Even though Amos targeted his PR for JTA we'd potentially have issues if users want to use JTA with other transaction models such as STM, LRA and future models (REST-AT, even XTS though I don't think we would propose that) because the property determines whether or not to start the generic recovery service.

If people insist on this change, as a workaround, then you should at least call the property disableXaRecovery with a default of true and a strong caveat in the javadoc - that way it looks more like a workaround for a bug rather than an optional feature. But I realise that if you're pushed for time and it's either this change or revert Amos' original PR then go for the workaround.

gsmet · 2022-09-20T13:15:41Z

So, first things first, this has been like that for ages (i.e. since the inception of Quarkus).

I don't have any problems changing the default in the future but we need to understand all the consequences (performances, usability, how to store this ObjectStore directory in a reliable way in test/dev/prod modes and in containers...).

My point of view is that once we have covered all the caveats, we can switch the default to true and thus it makes sense to keep the name as enableRecovery.

I'll create a proper issue with all the things that need coverage once I'm done with the releases.

mmusgrov · 2022-09-20T13:22:45Z

So, first things first, this has been like that for ages (i.e. since the inception of Quarkus).

true but now it's become a feature rather than a bug

I don't have any problems changing the default in the future but we need to understand all the consequences (performances, usability, how to store this ObjectStore directory in a reliable way in test/dev/prod modes and in containers...).

sure

My point of view is that once we have covered all the caveats, we can switch the default to true and thus it makes sense to keep the name as enableRecovery.

I'd still advise that you call it disableRecovery with a default value of true until such time as the caveats have been resolved.

I'll create a proper issue with all the things that need coverage once I'm done with the releases.

gsmet · 2022-09-20T13:27:03Z

I'd still advise that you call it disableRecovery with a default value of true until such time as the caveats have been resolved.

Yeah, I tend to avoid negative configuration properties as it makes it harder to figure out what it actually does when you end up with a double negative. In any case, we can revisit later and deprecate the old property if we end up having a strong case against it.

mmusgrov · 2022-09-20T13:30:50Z

Now it looks like recovery is deliberately offered as a legitimate choice even though the property is a workaround for a bug.

zhfeng · 2022-09-20T13:40:08Z

I can not see in what case user needs to disable recovery service. So it might be better to remove this option in the future if we resloved all the caveats.

gsmet · 2022-09-20T14:51:15Z

That's a valid option too :).

quarkus-bot bot added area/agroal area/narayana Transactions / Narayana labels Sep 17, 2022

zhfeng mentioned this pull request Sep 17, 2022

Narayana recovery manager service #26161

Merged

zhfeng commented Sep 17, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

gsmet requested changes Sep 19, 2022

View reviewed changes

zhfeng force-pushed the introduce_xa_enable_recovery branch from ca64491 to a9d2345 Compare September 19, 2022 10:09

zhfeng requested a review from gsmet September 19, 2022 11:03

gastaldi reviewed Sep 19, 2022

View reviewed changes

extensions/agroal/runtime/src/main/java/io/quarkus/agroal/runtime/DataSources.java Outdated Show resolved Hide resolved

zhfeng mentioned this pull request Sep 20, 2022

Don't merge - Revert "Improve Narayana recovery manager service and integrate it with agroal" #28003

Closed

gsmet approved these changes Sep 20, 2022

View reviewed changes

gsmet force-pushed the introduce_xa_enable_recovery branch from 1017eef to 0e5e5c0 Compare September 20, 2022 12:42

gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 20, 2022

gsmet mentioned this pull request Sep 20, 2022

Unused Narayana is started upon application start #27982

Closed

mmusgrov reviewed Sep 20, 2022

View reviewed changes

Introduce an option in narayana-jta for enabling XA recovery

31d3cb7

gsmet force-pushed the introduce_xa_enable_recovery branch from 0e5e5c0 to 31d3cb7 Compare September 20, 2022 13:02

gsmet added the triage/backport-2.13 label Sep 20, 2022

gsmet changed the title ~~Introduce enableXARecovery~~ Introduce enableRecovery in Nayarana extension Sep 20, 2022

gsmet merged commit 88b46b8 into quarkusio:main Sep 20, 2022

quarkus-bot bot added this to the 2.14 - main milestone Sep 20, 2022

quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Sep 20, 2022

gsmet modified the milestones: 2.14 - main, 2.13.0.Final Sep 20, 2022

gsmet removed the triage/backport-2.13 label Sep 20, 2022

zhfeng mentioned this pull request Sep 22, 2022

Upgrade Quarkus to 2.13.0.Final apache/camel-quarkus#4121

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce enableRecovery in Nayarana extension #28025

Introduce enableRecovery in Nayarana extension #28025

zhfeng commented Sep 17, 2022 •

edited by gsmet

Loading

zhfeng Sep 17, 2022 •

edited

Loading

gsmet Sep 19, 2022

gsmet Sep 19, 2022

This comment has been minimized.

This comment has been minimized.

gsmet left a comment •

edited

Loading

zhfeng commented Sep 19, 2022

gsmet commented Sep 20, 2022

mmusgrov Sep 20, 2022 •

edited

Loading

gsmet Sep 20, 2022

gsmet Sep 20, 2022

gsmet commented Sep 20, 2022

mmusgrov commented Sep 20, 2022

gsmet commented Sep 20, 2022 •

edited

Loading

mmusgrov commented Sep 20, 2022

gsmet commented Sep 20, 2022

mmusgrov commented Sep 20, 2022

zhfeng commented Sep 20, 2022

gsmet commented Sep 20, 2022

Introduce enableRecovery in Nayarana extension #28025

Introduce enableRecovery in Nayarana extension #28025

Conversation

zhfeng commented Sep 17, 2022 • edited by gsmet Loading

zhfeng Sep 17, 2022 • edited Loading

Choose a reason for hiding this comment

gsmet Sep 19, 2022

Choose a reason for hiding this comment

gsmet Sep 19, 2022

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

gsmet left a comment • edited Loading

Choose a reason for hiding this comment

zhfeng commented Sep 19, 2022

gsmet commented Sep 20, 2022

mmusgrov Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

gsmet Sep 20, 2022

Choose a reason for hiding this comment

gsmet Sep 20, 2022

Choose a reason for hiding this comment

gsmet commented Sep 20, 2022

mmusgrov commented Sep 20, 2022

gsmet commented Sep 20, 2022 • edited Loading

mmusgrov commented Sep 20, 2022

gsmet commented Sep 20, 2022

mmusgrov commented Sep 20, 2022

zhfeng commented Sep 20, 2022

gsmet commented Sep 20, 2022

zhfeng commented Sep 17, 2022 •

edited by gsmet

Loading

zhfeng Sep 17, 2022 •

edited

Loading

gsmet left a comment •

edited

Loading

mmusgrov Sep 20, 2022 •

edited

Loading

gsmet commented Sep 20, 2022 •

edited

Loading