Offer a way to configure the default transaction semantic for Narayana

[victor-h-costa]

Description

At the moment the default transaction semantic when using QuarkusTransaction is hardcoded to Semantic.REQUIRE_NEW. Allowing the semantic to be configured in application.properties would reduce the need for boilerplate code. The default transaction timeout is already configurable so am guessing this enhancement is fairly straightforward.

Implementation ideas

Property: quarkus.transaction-manager.default-transaction-semantic Type: DISALLOW_EXISTING, JOIN_EXISTING, REQUIRE_NEW, SUSPEND_EXISTING Default: REQUIRE_NEW

[yrodiere]

Not a fan of configuration properties that change the behavior of APIs so drastically, but in this case... It might be the way forward.

Let me explain: I would argue that REQUIRES_NEW is a very strange default and I would have expected simply JOIN_EXISTING. I say it's strange because it's inconsistent with other APIs: @javax.transaction.Transactional, for example, defaults to REQUIRED, which is similar to JOIN_EXISTING. I'm pretty sure Spring-specific annotations are similar.

But that's not the kind of things we can change without breaking backwards compatibility, unfortunately. If others agree that JOIN_EXISTING is more appropriate, maybe this configuration property you're suggesting could be used for a smooth transition: we change the default to JOIN_EXISTING, but allow users to set it back to REQUIRES_NEW. And someday, hopefully, we get rid of the configuration property.

That's just my opinion though, we'll need more input.

[quarkus-bot[bot]]

/cc @mmusgrov

[mmusgrov]

/cc @stuartwdouglas Do you recall your thinking process when choosing REQUIRES_NEW for the default Transaction Semantic?

[stuartwdouglas]

This is a programmatic transaction API, rather than a declarative one, basically it is supposed to be a replacement for usagesUserTransaction.begin() rather than @Transactional.

I don't think the different default compared to the annotations is an issue, as they are very different API's. I actually think that if you want JOIN_EXISTING semantics then @Transactional is probably a better fit for the use case.

[yrodiere]

I don't think the different default compared to the annotations is an issue, as they are very different API's.

I get that UserTransaction.begin() always starts a new transaction, and I agree QuarkusTransaction.begin() should do the same. They are, indeed, very similar to each other. And if I ask for a transaction to begin, then indeed, I expect a transaction to begin.

But are QuarkusTransaction.run/QuarkusTransaction.call more similar to UserTransaction.begin or to @Transactional? Based on the way they're used, at least, it becomes much more questionable. In particular, you have the concept of semantic/TxType for QuarkusTransaction.run/QuarkusTransaction.call/@Transactional, while you don't have that for UserTransaction.begin/QuarkusTransaction.begin(). If QuarkusTransaction.run and QuarkusTransaction.begin are both a replacement for QuarkusTransaction.begin, shouldn't they offer a similar feature set? So, at the very least, the lines are blurry.

My point is: regardless what the original intent behind QuarkusTransaction was, what matters is the expectations of someone using QuarkusTransaction.run/QuarkusTransaction.call for the first time, and calling that method from e.g. another method annotated with @Transactional. I know I would not have expected my existing transaction to be suspended implicitly.

I suspect suspending the existing transaction is also a more exotic use case than using the existing transaction, but I only have my own (short) experience as a business app developer to support that.

Anyway, if you still disagree, I think this warrants a warning in the documentation and javadoc, at least, and then we also need to answer @victor-costa-avancee 's request of a configuration property for the default behavior.

I actually think that if you want JOIN_EXISTING semantics then @transactional is probably a better fit for the use case.

I certainly agree @Transactional should be favored in general as it's simpler and less verbose, but I was (perhaps naively) under the impression that the declarative and programmatic approaches were just that, different approaches, different syntaxes. I would expect users to pick one syntax or the other based on the structure of their code or based on how dynamic they need transaction management to be, not based on their functional requirements (e.g. need for suspending a transaction or not)?

At least that's how I understand the current documentation, which just lists different approaches but does not mention use cases. Maybe that's another thing I should add to that documentation? Not sure I see what use cases are better addressed by one approach or the other, though.

[stuartwdouglas]

I will add some very simple examples to explain my thinking.

Say we have:

void doSomethig() {
  method1();
  QuarkusTransaction.run(() -> method2());
}

My expectation would be that method1() and method2() are not running in the same transaction. If I wanted them to be in the same TX I would just either group them both into the run method, or just use @Transactional. Basically the use case for this is to demarcate code into finer grained transactions than the method level, which is not really very useful if they just end up in the same transaction anyway.

[yrodiere]

Thanks @stuartwdouglas . I assume in your example, you expect doSomething to be called with an existing transaction already active, somehow? Okay, I understand. It would indeed be pretty confusing for the two methods to be suddenly executed in the same transaction when that wasn't the intent. I would have expected such code to be more explicit about the fact it needs a separate transaction, since QuarkusTransaction.run does not convey the meaning "I need my transaction to be separate", just "I need a transaction", but... okay.

That being said, there are other use cases and the opposite can be confusing as well:

class MyCdiClass {
  @Transactional
  void doSomething() {
    MyNonCdiClass myNonCdiObject = new MyNonCdiClass();
    myNonCdiObject.method1()
    myNonCdiObject.method2()
  }
}

// Cannot use @Transactional because it's not a CDI bean
// (e.g. because it's also potentially instantiated by
// third-party libraries and executed in background thread pools),
// so makes do with what it *can* use: QuarkusTransaction.
class MyNonCdiClass {
  public void method1() {
    QuarkusTransaction.run(() -> {...});
  }

  public void method2() {
    QuarkusTransaction.run(() -> {...});
  }
}

My expectation would be to method1 and method2 to be executed in the same transaction.

There's no way to make both assumptions work, though, so I guess we'll have to leave it at that.

Relatedly, I just checked Spring's TransactionTemplate, which addresses similar use cases, and it defaults to PROPAGATION_REQUIRED, which is equivalent to JOIN_EXISTING. IMO if we don't change our default, we need warnings and more documentation here, be it only to ease migration for people coming from Spring.

While I'm at it, I'll just bother you some more: can you please give your opinion (if any) on @victor-costa-avancee 's proposal of a configuration property to set the default semantic?

[stuartwdouglas]

I don't really like the idea of a property to control this behavior, I would rather add additional methods to give two explicit options.

Maybe we should start a thread on the dev list?

[yrodiere]

Maybe we should start a thread on the dev list?

Right, will do.

[FroMage]

I would also rather add new methods with explicit names.

[geoand]

I would also rather add new methods with explicit names.

+1

[yrodiere]

I would also rather add new methods with explicit names.

We did just that in #28903

So, while this ticket wasn't addressed directly (there's no configuration option to set the default transaction semantic), there is now an alternative solution. So, I'll close this ticket.

Thanks to everyone involved for the input!