Enhance LoopResources API in order to disable colocation

[pderop]

As discussed in #2781 GH issue, this PR proposes an enhancement in the LoopResources API in order to get an existing LoopResource, and mutate it to disable colocation.
(this enhancement is done on top of the main branch, so for 1.1.x reactor-netty versions).

So, to disable colocation on an existing LoopResources, using this PR, you can now call the new LoopResources.disableColocation()method that will return a LoopResources that will reuse the EventLoopGroup of the original LoopResources, but without colocation.

Here is an example of an R2DBC customizer which allows to reuse the default TcpResources without coloc:

@Component
public class R2DBCOptionsCustomizer implements ConnectionFactoryOptionsBuilderCustomizer {

    @Override
    public void customize(ConnectionFactoryOptions.Builder builder) {
        LoopResources loopResources = TcpResources.get().disableColocation();
        builder.option(MariadbConnectionFactoryProvider.LOOP_RESOURCES, loopResources);
    }
}

This could also be done in R2DBC driver implementations, for example, in the MariaDBConnectionConfiguration, it could be possible to change:

    this.loopResources = loopResources != null ? loopResources : TcpResources.get();

by:

    this.loopResources = loopResources != null ? loopResources : TcpResources.get().disableColocation();

PS: this allows to overcome a performance problem described in 2781 issue, however, it might be useful to also consider the reactor-pool warmup optimization proposed in reactor/reactor-pool#171, which might resolve the performance problem without having to do anything.

Fixes #2781

[pderop]

@violetagg , please do not review for the moment, let's wait to see what Mark thinks about this PR ?

[mp911de]

Does it make sense to have disableColocation on TcpResources as well?

[pderop]

The LoopResources.disableColocation is generic and can be applied to any default LoopResources, including the TcpResources (which implements the LoopResources interface), or the default HttpResources, or even the default UdpResources.

For example, in Mariadb driver, if users don't have provided any custom LoopResources, you can then configure the driver with a mutated version of the default TcpResources with colocation disabled like this in MariadbConnectionConfiguration:

    this.loopResources = loopResources != null ? loopResources : TcpResources.get().disableColocation();

The TcpResources inherits from the LoopResources.
it's important to note that you need to use the mutated LoopResources that is returned by the disableColocation() method (which does not alter the original LoopResources that remains unchanged and stay in colocated mode, so other components which are happy with this default mode won't be impacted, like the HttpClient).

If you want to share the default HttpResources with the R2DBC driver, you could also do it (in this case R2DBC will reuse the same event loop group from Http applications, but with coloc disabled (only for the R2DBC driver):

    this.loopResources = loopResources != null ? loopResources : HttpResources.get().disableColocation();

all this to say that the LoopResources.disableColocation() is generic and applies to either one of the existing default loop resources (HttpResources.get(), TcpResources.get(), or UdpResources.get()), or to even any custom colocated LoopResources created using one of the LoopResources.create(...) methods.

[pderop]

@violetagg , can you take a look please ?

[violetagg]

@pderop This interface LoopResources can have different implementations. The proposed change LoopResources.disableColocation() is specific to the implementation in Reactor Netty only (DefaultLoopResources, TcpResources etc). In other implementations (for example https://github.com/turms-im/turms/blob/b2a817058b45b5e8bbb8c0a6e5808e92bfbbb884/turms-server-common/src/main/java/im/turms/server/common/access/common/LoopResourcesFactory.java#L30) this may not be the case and colocation might not exist as a configuration at all.
I suggest that we provide a change that can fit also with other implementations if the change is on the interface level.

[pderop]

I was not aware about these other implements, ok I'll take a look and will think about it.

[pderop]

What do you think if I do not do the API change at the interface (instance) level, and instead of that, since we already have some create(...) methods which are using the ReactorNetty specific implementations (DefaultLoopResources, TcpResources, ...), then just add a new static create method like this in the LoopResources:

	/**
	 * Return a {@link LoopResources} that will share the same {@link EventLoopGroup} from an existing parent {@link LoopResources}.
	 * Disposing the returned {@link LoopResources} will also dispose the {@link EventLoopGroup} from the parent as well.
	 *
	 * @param parent the parent {@link LoopResources} whose underlying {@link EventLoopGroup} will be reused by the returned {@link LoopResources}
	 * @param colocate <code>false</code> will disable colocation from the parent {@link EventLoopGroup} if it was enabled in the parent.
	 *                 <code>true</code> means colocation will be enabled if it was disabled in the parent. The parent is not affected whether <code>true</code>
	 *                 or <code>false</code> is used.
	 * @return a LoopResources that will share the same {@link EventLoopGroup} from the specified parent, with or without colocation mode
	 * @since 1.1.9
	 */
	static LoopResources create(LoopResources parent, boolean colocate) {
		return new DefaultLoopResources(parent, colocate);
	}

This new static method is consistent with the other create methods that are depending on our ReactorNetty specfic implementations, and also, now we have a new
DefaultLoopResources(LoopResources parent, boolean colocate) constructor that is clearer than having a single constructor parameter DefaultLoopResources(LoopResources parent) that was implicitly disabling colocation.

would this be OK, and am I missing your point ?

[violetagg]

If we have in the future a request for changing other configurations how the API will look like?

[pderop]

You can ignore my last comment, indeed doing so will lead to a kind of bloated API, because in the future, if we have more new re-configurable configs, then we would end up
with many create method signatures like this, which would ruin the original cohesive feel of the API:

create(parent, boolean /* colocate */)
create(parent, boolean /* colocate */, boolean /* enable/disable new configuration1 */)
create(parent, boolean /* colocate */, boolean /* enable/disable new configuration1 */, boolean /* enable/disable new configuration2 */)
etc ...

in this case, a Builder API would be more appropriate.

But, I got back to your initial suggestion from #2842 (comment):

I suggest that we provide a change that can fit also with other implementations if the change is on the interface level.

and in the last commit, I revisited things, and it might be more aligned with what you have in mind, can you recheck ?

• first, I have reverted all changes from DefaultLoopResources, because in fact, it is not necessary to add complexity to this class.
• the LoopResources.disableColocation method is now using a helper class which is independant of our current Reactor-Netty LoopResources implementations (like DefaultLoopResources, TcpResources, ...)
• it should fit to other implementations like the one from turms;
  for example, if the turms LoopResourcesFactory has support for colocation and implements a onClient(boolean useNative) method that returns an EventLoopGroup decorated using LoopResources.colocate(EventLoopGroup group), then the disableColocation() method shoud fit ?

let me know ?

[violetagg]

@pderop This LoopResources.disableColocation is not a good solution. In the future if we need to change another configuration, will we expose again similar method on the interface level? I still think that something specific to a given implementation doesn't fit the interface level.

[pderop]

ok, so I'll continue to think about this.

[pderop]

I have attempted another approach in the last commit, can you check ?

• the LoopResoures public API (at the instance level) has not been modified

• in the PR, a new builder is proposed in order to create LoopResources implementations; it will avoid to have telescoping create methods. The builder also allows to create a wrapper LoopResources that can filter an existing parent LoopResources, in order to disable/enable functionalities from the parent, without affecting it.

• For the moment, the builder does not allow to "mutate" the builder that was used to build the parent (like it is done for example in the PooledConnectionProvider). This would probably necessitate to add a kind of "mutate" method in the LoopResources interface ...

• still to be done: deprecate the LoopResources.create(...) methods and fix all tests in order to use the new builder API instead of the create methods. Before doing this, just would like to know what you think ?

Example usages:

1. To create some LoopResources, you can now use the builder (the create methods are now based on the builder):

LoopResources customLoop = LoopResources.builder("loop-prefix")
    .workerCount(1)
    .daemon(false)
    .colocate(true)
    .build();

2. To disable the colocate mode without affecting the "customLoop", you can create a wrapped loopResources like this:

LoopResources wrapped = LoopResources.builder(customLoop)
      .colocated(false)
      .build();

The wrapped LoopResources will filter the parent customLoop and will disable colocate mode.

3. You can also disable colocate mode for the TcpResources default loop (this is actually the use case for Mark):

LoopResources wrapped = LoopResources.builder(TcpResources.get())
      .colocated(false)
      .build();

4. If in the future, for example, a new functionality is added with a new optional parameter, let's say "newOption":

LoopResources customLoop = LoopResources.builder("loop-prefix")
    .workerCount(1)
    .daemon(false)
    .colocate(true)
    .newOption(true)
    .build();

Then someone can disable colocation, as well as the new option like this:

LoopResources wrapped = LoopResources.builder(customLoop)
    .colocate(false)
    .newOption(false)
    .build();

The returned wrapped LoopResources will disable coloc and the newOption, but the parent customLoop will remain unaffected

by the way, I just figured out that it seems that Mark is able to do what he wants to do using the current LoopResources API (without any modification):

@Component
public class R2DBCOptionsCustomizer implements ConnectionFactoryOptionsBuilderCustomizer {
    @Override
    public void customize(ConnectionFactoryOptions.Builder builder) {
        LoopResources tcpUncolocated = (useNative) -> TcpResources.get().onServer(useNative);
        builder.option(MariadbConnectionFactoryProvider.LOOP_RESOURCES, tcpUncolocated);
    }
}

The above just wraps the default Tcp LoopResources with one that is simply redirecting onClient callbacks to the Tcp LoopResources onServer callback, which has the effect of simply disabling the colocation mode.
is this correct ?

[pderop]

I am closing this PR (see last comments from #2781 issue.)