Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable ReactiveRedisTemplate for multi-tenancy usage #2145

Closed
petromir opened this issue Aug 14, 2021 · 14 comments
Closed

Enable ReactiveRedisTemplate for multi-tenancy usage #2145

petromir opened this issue Aug 14, 2021 · 14 comments
Labels
type: enhancement A general enhancement

Comments

@petromir
Copy link
Contributor

Hi,

I would like to use ReactiveRedisTemplate in multitenant environment. In short, the option to use single ReactiveRedisTemplate which can connect to multiple Redis instances based on tenant values passed in reactive context.
Unfortunately, I can not connect the reactive context with the connection factory so I can create different connections (or using different connection pool) based on tenant value in the context.

One option is to override doInConnection, but it has private modifier. Changing it will allow me to provide custom logic about connection creation.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 14, 2021
@mp911de mp911de changed the title Change visibility of doInConnection method in ReactiveRedisTemplate Enable ReactiveRedisTemplate for multi-tenancy usage Aug 23, 2021
@mp911de mp911de added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 23, 2021
@mp911de
Copy link
Member

mp911de commented Aug 23, 2021

doInConnection is an internal method that handles resource allocation. What we should look for is how to enable ReactiveRedisTemplate for multi-tenancy usage. The first part of resource allocation is having a Mono emitting ReactiveRedisConnection. We could introduce a Mono<ReactiveRedisConnection> getConnection() method as extension point.

@petromir
Copy link
Contributor Author

Thanks a lot. When do you plan to tackle this?

@mp911de
Copy link
Member

mp911de commented Aug 24, 2021

Having a pull request would improve things in terms of timing so we could integrate it faster.

@petromir
Copy link
Contributor Author

I knew you gonna say that 😄

@petromir
Copy link
Contributor Author

petromir commented Aug 24, 2021

We could introduce a Mono getConnection() method as extension point.

I assume you mean in having this in ReactiveRedisTemplate?

@mp911de
Copy link
Member

mp911de commented Aug 24, 2021

Yes, exactly. A protected method that can be overridden by subclasses. Right now, the calls to obtain a connection return plain ReactiveConnection on the connection factory and are not suitable to access the reactor context. So solving the issue within the template would create the least impact.

@petromir
Copy link
Contributor Author

petromir commented Aug 24, 2021

Something like

private <T> Publisher<T> doInConnection(ReactiveRedisCallback<T> action, boolean exposeConnection) {

	Assert.notNull(action, "Callback object must not be null");

	return Flux.usingWhen(Mono.deferContextual(contextView -> getConnection(contextView, exposeConnection)), conn -> {
		Publisher<T> result = action.doInRedis(conn);

		return postProcessResult(result, conn, false);

 	}, ReactiveRedisConnection::closeLater);
}

protected Mono<ReactiveRedisConnection> getConnection(final ContextView contextView, boolean exposeConnection) {
	ReactiveRedisConnectionFactory factory = getConnectionFactory();
	ReactiveRedisConnection conn = factory.getReactiveConnection();
	ReactiveRedisConnection connToUse = preProcessConnection(conn, false);

	return Mono.just(exposeConnection ? connToUse : createRedisConnectionProxy(connToUse));
}

@mp911de
Copy link
Member

mp911de commented Aug 24, 2021

Mono.fromSupplier() is sufficient. One calling getConnection() that returns a Mono would expect deferred resource allocation. Otherwise, you already have a connection allocated and the only way to dispose it is by subscribing to the mono. Retaining fromSupplier(…) avoids such leaks in the first place.

@petromir
Copy link
Contributor Author

Then how I will be able to pass the context to getConnection()?

@mp911de
Copy link
Member

mp911de commented Aug 27, 2021

Since the method returns a Mono already, you can override the method by calling Mono.deferContextual(…) in your override.

@petromir
Copy link
Contributor Author

petromir commented Sep 8, 2021

You mean

private <T> Publisher<T> doInConnection(ReactiveRedisCallback<T> action, boolean exposeConnection) {

	Assert.notNull(action, "Callback object must not be null");

	return Flux.usingWhen(getConnection(exposeConnection), conn -> {
		Publisher<T> result = action.doInRedis(conn);

		return postProcessResult(result, conn, false);

 	}, ReactiveRedisConnection::closeLater);
}

protected Mono<ReactiveRedisConnection> getConnection(boolean exposeConnection) {
	ReactiveRedisConnectionFactory factory = getConnectionFactory();
	ReactiveRedisConnection conn = factory.getReactiveConnection();
	ReactiveRedisConnection connToUse = preProcessConnection(conn, false);

	return Mono.just(exposeConnection ? connToUse : createRedisConnectionProxy(connToUse));
}

Then in my overriding class

protected Mono<ReactiveRedisConnection> getConnection(boolean exposeConnection) {
	return Mono.deferContextual(contextView -> {
	    // Something that returns Mono<ReactiveRedisConnection> based on the value in the context
        }
)

@mp911de
Copy link
Member

mp911de commented Sep 8, 2021

Yes, exactly.

@petromir
Copy link
Contributor Author

@mp911de Please review my PR and let me know if I have to do some changes

@mp911de
Copy link
Member

mp911de commented Sep 13, 2021

Thanks, we're going to take this PR from here.

@mp911de mp911de added this to the 2.5.5 (2021.0.5) milestone Sep 13, 2021
mp911de pushed a commit that referenced this issue Sep 13, 2021
…) method.

A new method, called getConnection, is introduced to allow overriding of connection creation.

Closes #2145
Original pull request: #2162.
mp911de added a commit that referenced this issue Sep 13, 2021
Add author and since tags. Wrap connection allocation with fromSupplier(…). Move connection proxy decoration into doInConnection(…) as connection decoration isn't directly related to connection creation.

See #2145
Original pull request: #2162.
mp911de added a commit that referenced this issue Sep 13, 2021
Add author and since tags. Wrap connection allocation with fromSupplier(…). Move connection proxy decoration into doInConnection(…) as connection decoration isn't directly related to connection creation.

See #2145
Original pull request: #2162.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants