Use the jgaskins/redis shard

[jwoertink]

This PR doesn't change any sort of logic, but just the underlying shard from stefanwille/crystal-redis (OLD Redis) to jgaskins/redis (NEW Redis).

I believe the idea of #94 would be to allow you to choose which one of these (or other future implementations) you wanted to use, but I ran in to some issues which I decided to just pull the trigger and do the conversion.

Here's the main tricky points to why I needed this change for my app, and maybe others would too.

• OLD Redis doesn't support cluster, but my AWS setup on elasticache needs it. Although Mosquito works fine without it, I'm unable to drop down to the Redis level to utilize Redis on its own.
• OLD Redis uses an old pool shard which isn't actively maintained currently, and this may lead to issues of dead connections not being released properly
• NEW Redis uses the Crystal DB pool implementation which is a lot more active considering its use.
• OLD Redis uses class Redis for the namespace, but NEW redis uses module Redis for the namespace. This causes a collision meaning they both can't exist in the same project at the same time even as sub-dependencies of shards you can't control. If Cable CR moves to NEW Redis, then all apps using both Mosquito, and Cable will instantly break.
• Our app has a redis reconnect issue if redis dies. NEW Redis seems to handle this by automatically reconnecting.
• Noting the 4th point, Mosquito would need some way to test all of this also to include the Demo... That means that one of these shards has to be a dependency at some point (maybe just development?), but back to that point, they both can't exist at the same time. Which means that Swappable redis configuration #94 won't really matter unless Mosquito creates a fake redis-like mock system that becomes swappable, and in the interest of time, this seemed easier.

The only major downside I've ran in to is OLD Redis was really nice about "This returned a String, so here's a String"... NEW Redis has this Value that is a recursive alias. This makes casting things a little harder since you don't quite know what something is which is why there's a lot of .as(Something)....

EDIT: Before switching to this PR, I was having to reboot my app daily due to dropped redis connections with OLD redis. Using this PR and NEW Redis, in production, it's been a few days with no issues.

[jwoertink]

I've been working with @jgaskins on this, and there's still a few small things being updated before the next release. This will need to wait until the next release before being locked down.

[jwoertink]

The Redis::Client is pooled by default in this shard.

[jwoertink]

😬

[robacarp]

Wow this is great. Thank you. I'm not opposed to this change, but it'll have to be a major point release so it doesn't nuke anyone that might depend on using the stefanwillie shard for some other uses.

The .as() glitter sprinkles this creates isn't my favorite, and I might be tempted to put in a wedge class to take care of that all in one place. However I don't think that's necessary in order to get this merged, it can be something I clean up later.

[robacarp]

The Cable CR pull to match this

[mamantoha]

I think the best solution will be if mosquito will not directly depend on any of these shards. Backend implementation for stefanwille/crystal-redis and jgaskins/redis can live in their own repositories.

And then something like this:

Mosquito::Backend.register_backend("old_redis", Mosquito::RedisBackend)

Mosquito.configure do |settings|
  settings.backend = "old_redis"
end

This will fix the main issue that they both can't exist in the same project at the same time.

[robacarp]

@mamantoha I agree. I toyed around with that idea in #94. It's not untenable, but it's fiddly and excessively verbose. Crystal doesn't really have good tooling around this problem.

If a clamor of folks that must use stefanwillie/redis or else they'd need to pay a cost of hundreds of man hours to switch existed, I think it would be worth exploring. That's why this proposal exists, to solicit feedback from anyone who is willing to participate.

[mamantoha]

@robacarp I'm ok with the current dependency on stefanwillie/redis because I don't use jgaskins/redis in my project (https://github.com/mamantoha/shards-info). But if you change dependency I will have some issues. Because I use some shards which have stefanwillie/redis as sub-dependencies.
Actually, you don't need to maintain both backends. Any of them is ok if it's not the direct dependency.
In case you select jgaskins/redis, I can maintain the backend based on stefanwillie/redis .

[jwoertink]

Ah, yeah, that's the tricky part. If you have other projects that also depend on the stefanwillie/redis then this will for sure break your project.

I'd imagine if we went the other route, then Mosquito wouldn't be able to include any redis shard at all in the project. This means that any spec testing this stuff would all have to be mocked through an interface. Then there would need to be another shard created like mosquito-cr/stefanwillie-redis-mosquito-adapter 😂 Anyone using this would then need to include both shards. That will lead to some other challenges by having to keep an extra dependency up-to-date. It would also mean I would have to also write a separate adapter.

With that said, it's not a bad thing to go that route, but I think more of a time/energy constraint. I have to deploy to production today, so I'll be using my fork regardless. Maybe someone would be willing to tackle that approach? I'd be cool with that too.

[robacarp]

@mamantoha Thanks for the context. It's complicated for sure. Would you mind sharing the dependencies which you have that also depend on stefan's redis?

[mamantoha]

@robacarp https://github.com/neovintage/kemal-session-redis

It's a pretty simple library. So in case, you will accept this PR I prefer to maintain a fork of kemal-session-redis rather than a fork of mosquito 😄

[mamantoha]

@robacarp FYI. You can check all repositories dependent on stefanwille/crystal-redis here https://shards.info/github/stefanwille/crystal-redis/dependents

[jgaskins]

Just adding some clarifications (and following the OLD/NEW convention in this PR for consistency):

• OLD Redis uses an old pool shard which isn't actively maintained currently, and this may lead to issues of dead connections not being released properly
• NEW Redis uses the Crystal DB pool implementation which is a lot more active considering its use.

I worked with the Crystal core team to make DB::Pool usable as a generic connection pool specifically because of things ysbaddaden/pool wasn't solving. DB::Pool is thread-safe, elastic, and can eagerly spin up a minimum pool, for example.

The simplicity of ysbaddaden/pool is awesome to show what you can accomplish in Crystal with so few lines of code, but DB::Pool is just better as a production-grade connection pool.

• Our app has a redis reconnect issue if redis dies. NEW Redis seems to handle this by automatically reconnecting.

TIL OLD Redis isn't self-healing. Applications shouldn't have to micromanage encapsulated TCP connections being severed and, after a cursory read through the pertinent parts of the code, I'm not sure that's something you can do at all with that client. I also can't find a way for OLD Redis's PooledClient to discard a closed connection (fun fact: DB::Pool can handle this out of the box) or reconnect.

I've seen so many disconnections with both ElastiCache and Heroku Redis in the past that I find it odd that this hasn't come up more with that shard.

The only major downside I've ran in to is OLD Redis was really nice about "This returned a String, so here's a String"... NEW Redis has this Value that is a recursive alias. This makes casting things a little harder since you don't quite know what something is which is why there's a lot of .as(Something)....

This can be streamlined in NEW Redis trivially. It likely just needs some type overrides added in Redis::Connection for commands that have been added recently. May need to be added to Cluster, as well.

The Value union type was crucial for extension. It's what allows extending Pipeline, Transaction and the various first-party Redis modules like Redis::TimeSeries, Redis::FullText, and Redis::JSON to be supported without any changes to the core code — beyond a simple monkeypatch. Because it's implemented the way it is, you can extend the client for any Redis module and you will at worst have to do some typecasting and/or provide wrappers to transform nested arrays into more Crystal-friendly data structures. Just needs some additional refactoring to make it easier to use with Client, Connection, and Cluster. I'll post an issue on the repo for that.

[robacarp]

This is the direction for mosquito to go in. I'd like to see jgaskins/redis#32 go in first, because the .as() business is my biggest hiccup with the switch. That will also ease the pain of any other shards which depend on redis which need to switch over.

@mamantoha I appreciate that this will put you in a position to fork, modify and/or maintain the Kemal session shard, and I'm sorry that is the case. As you said, the library complexity is an order of magnitude less.

My intent is that the second version of mosquito (i.e. v1.0.0) can launch with jgaskins redis, but the rc1 will continue to point at stefanwillie's implementation.

[mamantoha]

@robacarp It'll be no problem at all, believe me.

[robacarp]

FYI this has been merged in #106