Introduce QuarkusTransaction#runner

[yrodiere]

Addresses the problem discussed on the mailing list, namely the fact that QuarkusTransaction#run()/QuarkusTransaction#call have a default semantic that is not necessarily obvious.

Note this does not implement the solution requested in #28579, so merging this PR should not close that issue as "fixed". Maybe we should close it as "not planned", though, since it probably won't be relevant anymore.

The solution we discussed is simple: we expect the semantic to be specified explicitly.

To specify the semantic explicitly, we originally decided to create "one method per semantic". But... it's not that simple. There are actually 4 methods, all slight variations of the same concept:

• Accepting Runnable vs. Callable
• With a RunOptions parameter vs. without.

If we duplicated that for every semantic, we would end up with a lot of variations:

• 4 different semantics
• Accepting Runnable vs. Callable
• With a RunOptions parameter vs. without.

We're already at 4*2*2 = 16 methods. I'm concerned that could be even more confusing that non-obvious defaults.

So... I took a slightly different approach. We still have an explicit semantic, but it's a parameter of the method. And, for reasons I'll detail below, I also replaced RunOptions with a new concept of TransactionRunner (quite similar to Spring's TransactionTemplate, as it turns out).

There is now a single entrypoint:

static TransactionRunnerOptions runner(TransactionSemantic semantic);

And instead of this:

        QuarkusTransaction.run(() -> {
            //do work
        });

        int result = QuarkusTransaction.call(QuarkusTransaction.runOptions()
                .timeout(10)
                .exceptionHandler((throwable) -> {
                    if (throwable instanceof SomeException) {
                        return RunOptions.ExceptionResult.COMMIT;
                    }
                    return RunOptions.ExceptionResult.ROLLBACK;
                })
                .semantic(RunOptions.Semantic.SUSPEND_EXISTING), () -> {
            //do work
            return 0;
        });

Users will now have to do this:

        QuarkusTransaction.runner(TransactionSemantic.REQUIRE_NEW).run(() -> {
            //do work
        });

        int result = QuarkusTransaction.runner(TransactionSemantic.SUSPEND_EXISTING)
                .timeout(10)
                .exceptionHandler((throwable) -> {
                    if (throwable instanceof SomeException) {
                        return TransactionExceptionResult.COMMIT;
                    }
                    return TransactionExceptionResult.ROLLBACK;
                })
                .call(() -> {
                    //do work
                    return 0;
                });

The syntax is slightly more verbose, but then that's to be expected if we want to be more explicit.

Alternatively, we could have gone with QuarkusTransaction.run(Semantic, RunOptions, Runnable) and deprecated/removed the .semantic(...) setter from RunOptions. However:

• We would still need 4 different methods (+ the legacy methods with non-obvious default semantic).
  With the solution in this PR, there's a single entry point and users can discover their options step by step (first the semantics, then the options, then the various types of callbacks).
• The syntax would be, arguably, even more complex. See the example from the documentation:

        int result = QuarkusTransaction.call(
                RunOptions.Semantic.SUSPEND_EXISTING,
                QuarkusTransaction.runOptions()
                        .timeout(10)
                        .exceptionHandler((throwable) -> {
                            if (throwable instanceof SomeException) {
                                return RunOptions.ExceptionResult.COMMIT;
                            }
                            return RunOptions.ExceptionResult.ROLLBACK;
                        }), () -> {
                            //do work
                            return 0;
                        });

  vs. simply:

        int result = QuarkusTransaction.runner(TransactionSemantic.SUSPEND_EXISTING)
                .timeout(10)
                .exceptionHandler((throwable) -> {
                    if (throwable instanceof SomeException) {
                        return TransactionExceptionResult.COMMIT;
                    }
                    return TransactionExceptionResult.ROLLBACK;
                })
                .call(() -> {
                    //do work
                    return 0;
                });

• Until we finally remove the deprecated semantic(...) setter from RunOptions we would end up allowing weird code such as:

        int result = QuarkusTransaction.call(
                RunOptions.Semantic.SUSPEND_EXISTING,
                QuarkusTransaction.runOptions()
                        .semantic(RunOptions.Semantic.REQUIRE_NEW), // override! Which semantic will win is anyone's guess.
                        () -> {
                            //do work
                            return 0;
                        });

We could theoretically go one step further and actually introduce one method per semantic (since the other variations are now hidden in the TransactionRunner DSL). I'm not sure how useful that would be, but let me know if you think that's necessary:

        // Instead of, or on top of:
        QuarkusTransaction.runner(TransactionSemantic.REQUIRE_NEW).run(() -> {
            //do work
        });
        // We would have:
        QuarkusTransaction.requiringNew().run(() -> {
            //do work
        });
        QuarkusTransaction.joiningExisting().run(() -> {
            //do work
        });
        QuarkusTransaction.suspendingExisting().run(() -> {
            //do work
        });
        QuarkusTransaction.disallowingExisting().run(() -> {
            //do work
        });

cc @FroMage @geoand

[quarkus-bot]

/cc @Sanne, @gsmet

[geoand]

I personally like it a lot, but @FroMage is the API expect as far as I am concerned, so I'll defer to him on whether this is the best approach or not.

[gsmet]

@Sgitario could you have a look at the failures above ^. From what I can see, this is failing since the merge of the Kubernetes dev services?

[Sgitario]

@Sgitario could you have a look at the failures above ^. From what I can see, this is failing since the merge of the Kubernetes dev services?

Yep, these failures should be related to the changes in #28305.
Since these tests are not expected to have the Dev Services for Kubernetes Client, I've disabled this dev service: #28943. Let's wait for the CI and double-check that there are no more affected tests.

[yrodiere]

Rebased on latest main.

@FroMage any opinion on this? It seems like @geoand would like to hear from you :)

I personally like it a lot, but @FroMage is the API expect as far as I am concerned, so I'll defer to him on whether this is the best approach or not.

[FroMage]

Hi. So, first we perhaps need to clear up the word semantic vs semantics. I always heard from people that corrected me that it was the noun we should use semantics and not the adjective semantic, but this is more a question for native speakers such as @gavinking or @geoand :)

Second, in terms of UX, if I go to my IDE and start typing QuarkusTransaction.runner( and call completion, it tells me it needs an enum, so I have to type the enum name before I can get meaningful completion options of what the semantics can be. To save 6 methods, this feels an unnecessary burden. Perhaps this is just my shitting IDE, and others will actually complete the argument directly?

Otherwise, I prefer the last proposed option where I can do QuarkusTransaction. and hit complete and get meaningful results such as requireExisting, suspend, whatever, and get rid of the enum. All the other options are fine in the builder (runner).

WDYT?

[geoand]

I always heard from people that corrected me that it was the noun we should use semantics and not the adjective semantic, but this is more a question for native speakers such as @gavinking or @geoand

My "nativeness" is totally based on speaking the language growing up, I don't have any formal training it - that means that I only know what sounds "right" to me and occassionaly this does not correspond to proper use of the language.

With that caveat, I can't think of a situation where I would semantic instead of semantics

[yrodiere]

Thanks for the feedback!

first we perhaps need to clear up the word semantic vs semantics

I can't think of a situation where I would semantic instead of semantics

Done: converted everything I could find to "semantics"

Perhaps this is just my shitting IDE, and others will actually complete the argument directly?

IDEA seems to, but that seems a bit random. Regardless, that's a valid argument.

Otherwise, I prefer the last proposed option where I can do QuarkusTransaction. and hit complete and get meaningful results such as requireExisting, suspend, whatever, and get rid of the enum. All the other options are fine in the builder (runner).

Done. Though I went with -ing methods (requiringExisting, joiningExisting, ...), so as not to make it seem like the method, in itself, actually requires an existing transaction; otherwise someone will try QuarkusTransaction.requireExisting(); and be confused when it does nothing... I also kept .runner(TransactionSemantics) because it can be useful to build tools around these methods without those tools having to declare their own enum and use a switch.

Does this look better?

[yrodiere]

Alright, looks like I went on leave just after I updated this PR and it was forgotten about :)

@geoand @FroMage Can one of you please approve the PR? Thanks!

[geoand]

Welcome back ☺️

[quarkus-bot]

Failing Jobs - Building 00c4167

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✔️	JVM Tests - JDK 17
✖	JVM Tests - JDK 17 MacOS M1	Set up runner	⚠️ Check →	Logs	Raw logs
✔️	JVM Tests - JDK 18
⌛	MicroProfile TCKs Tests	Verify	⚠️ Check →	Logs	Raw logs

[yrodiere]

From what I can see, the Mac OS failure is unrelated to this PR.

Merging, thanks!

[yrodiere]

I added a section to the migration guide: https://github.com/quarkusio/quarkus/wiki/Migration-Guide-2.16#quarkus-transaction