Add a temporary config property to allow multiple resources

[gsmet]

The plan is to remove this property in the future but we need to provide a way for people to update to latest security fixes without having to significantly adjust their applications.

When we remove it, we need to document this breaking change properly and also provide guidance on how to fix the most common issues that could be encountered due to this breaking change.

TODO:

• Document in the 3.8 migration guide (and maybe the other versions too) - the migration guide should clearly state the error message so that people can find it with the error message
• I wonder if we should have a paragraph in the documentation too, again with the explicit error message and strategies on how to fix the apps (part of this section should stay in 3.11 as the plan is to drop the property).

Fixes #39283

[Sanne]

Looks great to me.

I'm not 100% sure if we should really remove this in the future - yes we should be clear that we don't recommend (nor support!) this option, and perhaps even link to an explainer (a Narayana article/blog would be great to explain to people why this is important!), but I'd be willing to be pragmatic about it: some people might appreciate that, especially while creating POCs.

[gsmet]

/cc @mmusgrov @zhfeng

[zhfeng]

Thanks @gsmet - LGTM !

[mmusgrov]

The code changes are good but I have requested additional text for the javadoc for the new config property.

[mmusgrov]

Looks great to me.

I'm not 100% sure if we should really remove this in the future - yes we should be clear that we don't recommend (nor support!) this option, and perhaps even link to an explainer (a Narayana article/blog would be great to explain to people why this is important!), but I'd be willing to be pragmatic about it: some people might appreciate that, especially while creating POCs.

This was my concern, once it's in there will be reluctance to remove it. It is useful for development but I haven't been able to come up with any other use-case.

[gsmet]

I included @mmusgrov 's changes and changed the wording a bit for the removal, asking users to open issues if they see a valid use case. That way we will be able to take an educated decision in the future.

[gsmet]

@mmusgrov I did some further tests with the reproducer and I encountered the following warning twice:

2024-04-30 14:07:03,796 WARN [com.arj.ats.arjuna] (Camel (camel-1) thread #1 - scheduler://read-clean-write) ARJUNA012141: Multiple last resources have been added to the current transaction. This is transactionally unsafe and should not be relied upon. Current resource is LastResourceRecord(XAOnePhaseResource(io.agroal.narayana.LocalXAResource@68e1bf3d))

Is it a warning that will be logged for each transaction? Because if so I don't think it will fly as we will flood the log.

[gsmet]

OK, I filtered your warnings and added a warning of our own saying:

2024-04-30 14:24:05,908 WARN [io.qua.nar.jta.run.NarayanaJtaRecorder] (main) Setting quarkus.transaction-manager.allow-unsafe-multiple-last-resources to true makes adding multiple resources to the same transaction unsafe.

I think this should be good enough. It will be displayed only once when the application is started.

[Sanne]

I think this should be good enough. It will be displayed only once when the application is started.

Just curious, you really mean it's going to be shown once? I didn't know we had that capability.

[mmusgrov]

OK, I filtered your warnings and added a warning of our own saying:

2024-04-30 14:24:05,908 WARN [io.qua.nar.jta.run.NarayanaJtaRecorder] (main) Setting quarkus.transaction-manager.allow-unsafe-multiple-last-resources to true makes adding multiple resources to the same transaction unsafe.

I think this should be good enough. It will be displayed only once when the application is started.

@gsmet There is another property to disable it:

`arjPropertyManager.getCoreEnvironmentBean().setDisableMultipleLastResourcesWarning`

But the warning should only be issued once for each last resource, since you have two such last resources I'd expect to see it printed twice. I think it's valid to report the warning on each such attempt to register a last resource, ie I don't think it should be swallowed.

[mmusgrov]

ie I think it's valid to report the warning on each attempt to register a last resource (we only report it once per offending resource anyway), ie I don't think the second one should be swallowed since the user needs to know that he's heading into choppy waters with the second resource.

[gsmet]

Ah OK, if it's issued only once then it's all good. I removed the two additional commits then.

[mmusgrov]

Actually I think it may still be printed on each transaction unless that property (setDisableMultipleLastResourcesWarning) is set. The check for whether to report a warning is here. Would you be able to check your reproducer with two transactions?

[mmusgrov]

We also print a warning during start up if setAllowMultipleLastResources(true) has been called.

[mmusgrov]

Similarly if setDisableMultipleLastResourcesWarning(true) has been called. As you can probably tell, we are keen to let the user know he's using multiple last resources.

[Sanne]

@Sanne :

I've made some suggestions to clarify the docs about this; in particular it seems to suggest that the defaults don't allow to use multiple XA resources which isn't exact.

Well technically XA isn't enabled by default, so the documentation is exact. If you want more datasources, you need extra configuration: enable XA, or use workarounds. That's why the documentation is structured this way.

I can rephrase the first sentence like this if you want:

By default, a transaction may include at most one non-XA datasource.

Yes please, it's much more precise and clear.

But I'd keep the same structure, as I suspect the people needing this section the most will not be familiar with XA. WDYT?

I'm surprised that you'd hink that most users will not be familiar with XA. Sure, I don't expect many to be expert in it, but at least being familiar with the notion should be a requirement to make applications.

[yrodiere]

but at least being familiar with the notion should be a requirement to make applications.

Sanne, people struggle with basic SQL.

EDIT: Besides, I'm not worried about "most users", but rather about "the minority who doesn't understand this stuff and will need this documentation the most".

[yrodiere]

I addressed @Sanne 's comments and split the warn option.

So here's what we have now:

• The property is named quarkus.transaction-manager.unsafe-multiple-last-resources
• It has four possible values:
  • allow: allow unsafe multiple last resources, no warning per offending transaction
  • warn-first: allow unsafe multiple last resources, one warning on the first offending transaction only, no warning after that
  • warn-each: allow unsafe multiple last resources, one warning log per offending transaction
  • fail: fail on unsafe multiple last resources
• It will log a warning on startup if explicitly set to allow or warn-first or warn-each.
• It defaults to fail in this PR.
• The plan is to make it default to warn-first in 3.8, which means no log on startup, but one per offending transaction. People will be able to set it to allow explicitly to suppress the warning on the first transaction, but will get a warning on startup (since they set the property explicitly).

@gsmet you can test this using my fork of the reproducer, which processes batches of 2 items, so it'll open multiple transactions: https://github.com/yrodiere/quarkus-camel-transactions/tree/fix

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 3b008a8.

✅ 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.

⚠️ There are other workflow runs running, you probably need to wait for their status before merging.

[quarkus-bot]

Status for workflow Quarkus CI

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

✅ 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.

I think Yoann addressed your concerns and we can tweak the doc further later if needed. I'm trying to wrap up 3.11.0.CR1 and 3.10.1.

[mmusgrov]

@shawkins @yrodiere Reporting mixed outcomes (first resource commits and the second one rolls back) from XA unaware participants requires some invasive changes to ArjunaCore so that we can support crash recovery and it will take us a while to design the changes. But we can detect such heuristic outcomes so would it be sufficient, in the short term, to log a WARNing instead and longer term we'd report the HeuristicMixedException?

Note also that the probability of mixed outcomes is much higher than LRCO because the window between committing the first and second resources includes a network call so that window is quite long compared to the LRCO window of exposure.

[yrodiere]

I'll let @shawkins answer about the HeuristicMixedException, personally I'm not familiar with that. I think @shawkins wanted to open a dedicated issue about the specific needs of Keycloak in this area?

As far as I'm concerned, logging a warning in case of dodgy rollbacks seems like a reasonable first step, at least better than no information at all. I'm not sure it will help a lot but it will at least help debugging.

Though IMO solving #40431 (getting a clear exception in case of multiple non-XA datasources in a single transaction) would be more beneficial to Quarkus users in general -- those who didn't enable dangerous settings.

[shawkins]

I'll let @shawkins answer about the HeuristicMixedException, personally I'm not familiar with that. I think @shawkins wanted to open a dedicated issue about the specific needs of Keycloak in this area?

The main concern is around making users aware / documenting that a mixed situation has occurred. If that's via log, it will work as a first step.