search

stefanwille / crystal-redis

Full featured Redis client for Crystal
MIT License
380 stars 61 forks source link

convert select() to no-op if the current database is the same as target one #66

Closed DRVTiny closed 4 years ago

DRVTiny commented 6 years ago

select() with the same database number as the already selected one leads to no exchange with the Redis server

stefanwille commented 6 years ago

Hi @DRVTiny, thanks for your PR.

To me this looks like an optimization for a case that has little practical performance impact?

DRVTiny commented 6 years ago

I don't think so. For example, if you store different kind of serialized objects in different databases - you either have to use N redis connections or 1 connection but with frequently select's between databases. In common case you have to compare database property of redis connector and number of target database manually in your - or crystal-redis driver can do this for you - maybe in some more sophisticated way. select() is not too time consuming operation in 99% of scenarious, but in library code it will be reasonable to think about rest 1%, because sockets may be slow and excessive commands sensed through when you can avoid this - may leads to additional delays without any reasons: it is very simple to check whether you need to switch from one database to another or no. So why not to do this? I don't think that using one redis connector in several parallel threads will be more common scenario than those i've describer earlier...

пн, 25 июня 2018 г., 20:06 Stefan Wille notifications@github.com:

Hi @DRVTiny https://github.com/DRVTiny, thanks for your PR.

To me this looks like an optimization for a case that has little practical performance impact?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stefanwille/crystal-redis/pull/66#issuecomment-400025093, or mute the thread https://github.com/notifications/unsubscribe-auth/AB7q9xx97W2LIT1xmwLikL3I4TbbwQ0iks5uARiQgaJpZM4U2Y5v .

DRVTiny commented 5 years ago

stefanwille, this pull request includes setting database as Redis instance attribute as well. Because of some unobvious bugs in Redis driver code, i've got, for example, connections in Redis::PooledClient pool where select() was not called for some very strange reasons. To debug this case i need to know current @database value... somehow. It is normal practice to store different kind of Redis objects in different databases. This possibility is a basic Redis functional, so i need to be possible to use it in my Crystal code.

stefanwille commented 4 years ago

Merged, thanks!