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

Allow auto flushing control on connection pool #162

Closed
CodingFabian opened this issue Dec 16, 2015 · 10 comments
Closed

Allow auto flushing control on connection pool #162

CodingFabian opened this issue Dec 16, 2015 · 10 comments
Labels
for: team-attention An issue we need to discuss as a team to make progress type: feature A new feature
Milestone

Comments

@CodingFabian
Copy link
Contributor

I am curious why autoflush control is not allowed on a pooled connection.
Is it to safeguard agains unexpected auto flush modes on the pooled connection?
In our use case we would like to use a pool of connections, but all usage actually is async and even further all usage is actually to "fire and forget". so we would prefer use manual flushing because the code using a pooled connection will perform a number of batched queries.

Would there be any actual problems in doing so?

@mp911de
Copy link
Collaborator

mp911de commented Dec 16, 2015

Hi @CodingFabian

setAutoFlushCommands is disabled for not affecting the connection state. Some other commands are blocked as well (select, auth, quit). This scheme is however not aligned with the transactional commands (MULTI/EXEC).

TBH, connection pooling is not the best feature of lettuce. There are various ways how to deal with connection pooling and how users want to use pooling hence my strategy is to support Redis as good as possible but leave pooling up to the user.

@mp911de mp911de added the for: team-attention An issue we need to discuss as a team to make progress label Dec 16, 2015
@CodingFabian
Copy link
Contributor Author

Fair enough, that is reasonable.
The reason I like the connection pool you provide is that it is easy to use. Most importantly it "repairs" broken connections by itself. (it validates by using the isOpen check)
Because that is redis connection specific, i actually like that the pool takes care of that for me.

There are various ways for me to bypass the protection, so thats not an issue for me.

Another option would be that I can provide my own connection factory for the pool, so i can control the intitial state of a connection. This is currently not doable because of visibility.

@mp911de
Copy link
Collaborator

mp911de commented Dec 16, 2015

You mean to make createFactory public/protected? It's way more required to do that because the asyncPool/pool methods in RedisClient do multiple things (e.g. ensure the resources are free'd when the client is shut down).

@CodingFabian
Copy link
Contributor Author

Maybe hook into here:
https://github.com/mp911de/lettuce/blob/master/src/main/java/com/lambdaworks/redis/RedisClient.java#L291
Allow using a custom RedisConnectionProvider. That way I could execute state changing commands before adding the connection to the pool.

@mp911de
Copy link
Collaborator

mp911de commented Dec 16, 2015

I'd rather allow the state-changing commands than modify the API. Gives more responsibility to the user and would solve your case.

@CodingFabian
Copy link
Contributor Author

Thats up to you, I just wanted to present my use case. Feel free to implement it any way or not do it at all. Thanks for your work on this nice library!

@mp911de
Copy link
Collaborator

mp911de commented Dec 16, 2015

Thanks for pointing this out. Not (yet) convinced to fully removing pooling.

@mp911de mp911de added the type: feature A new feature label Dec 16, 2015
@mp911de mp911de added this to the Lettuce 3.4 milestone Dec 16, 2015
@CodingFabian
Copy link
Contributor Author

I never suggested to remove pooling? I suggested to make it possible to modify the state of a pooled connection, either at creation or usage time

@mp911de
Copy link
Collaborator

mp911de commented Dec 16, 2015

All good, sorry for confusing you. The leaky pooling implementation comes just more often across my way and I need to do something about it.

@mp911de
Copy link
Collaborator

mp911de commented Dec 22, 2015

The commands auth, select, quit, and setAutoFlushCommands may now be called on pooled connections.

@mp911de mp911de closed this as completed Dec 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: team-attention An issue we need to discuss as a team to make progress type: feature A new feature
Projects
None yet
Development

No branches or pull requests

2 participants