[lavrukov]

The idea is to address the ScanBuilderImpl problem while preserving backward compatibility and keeping scan-specific configuration available only after calling scan().

This keeps scan-specific logic out of StdTxManager and StdTxOptions, apart from the scan() entry point. Everything else lives in ScanTxManager and ScanTxOptions, which are now simple wrappers that delegate to the underlying transaction manager and its options.

As a bonus, TxManager now has 15 (!!) fewer methods that implementations must provide, making it much easier to write wrappers.

If we agree on this change, readOnly() will be next.

[nvamelichev]

❌ Tests failed:

[ERROR] Failures: 
[ERROR]   TxOptionsTest.testTimeoutOptionsCantAcceptNullTimeout Expected exception: java.lang.NullPointerException
[ERROR]   TxOptionsTest.testTimeoutOptionsCantBeChangedToNull Expected exception: java.lang.NullPointerException

[nvamelichev]

This solution looks weirdly elegant... I have some minor suggestions though. And we need to write some tests for ScanTxManager with typical wrapping transaction managers that, primarily, do extra metrics/tracing/instrumentation - to make sure that this logic is still invoked and works properly. Especially important is the case of tracing: we need to write a "scan_query" tag into the trace indicating that this was a scan query.

Currently we do something like that (greatly simplified):

public final class TracingTxManager extends DelegatingTxManager {
    // <...>

    @Override
    protected <T> Supplier<T> wrapTxBody(Supplier<T> supplier) {
        return () -> runWithTracing(supplier);
    }

    private <T> T runWithTracing(Supplier<T> supplier) {
        var tx = Tx.Current.get();
        var opts = tx.getRepositoryTransaction().getOptions();

        try (var span = // ...
            .setTag("tx.isolation_level", opts.isScan() ? "SCAN" : opts.getIsolationLevel().name())
            .setTag("tx.scan", String.valueOf(opts.isScan()))
            .startSpan()
        ) {
            return supplier.get();
        }
    }

TxOptions.isScan() must return the correct thing here (true) when someone calls TracingTxManager.scan().run(...)!

[nvamelichev]

To use a Markdown-style doc comment, write ///, not // :-)

[nvamelichev]

Naming: Maybe TxOptions.DataQuery for ordinary data query options and TxOptions.ScanQuery for scan query options?

[nvamelichev]

can delegate be anything else than StdTxOptions?

[nvamelichev]

Style: Preconditions.checkState(options instanceof TxOptions.ScanQuery scanOptions, "error msg...") reads quite a bit better than !(options instanceof ...). I'd use Preconditions.checkState() if it also does the pattern-matching magic materializing an already-casted scanOptions variable.

[nvamelichev]

maybe something like scanOptions = options.withDelegate(newOptions); where withDelegate() is a package-private method unavailable to library users?

[nvamelichev]

Maybe just return this;? I can imagine some generic code handling TxManager and wanting to ensure that a scan query is performed by calling scan() without checking instanceof (an instanceof check would be incomplete anyway; there are lots of DelegatingTxManager implementations out there, sometimes multi-wrapping each other).

[nvamelichev]

Let's maybe move this check-and-cast into ScanTxManager's constructor?