Add async driver.

[ioquatix]

This is a work in progress to add support for async.

[ioquatix]

assert (Time.now - start_time) <= opts[:timeout]

This test should be using monotonic time.

[ioquatix]

Would you like me to add some documentation, e.g. to README?

[ioquatix]

@byroot do you think you have time to give me feedback and help merge this PR?

[ioquatix]

@antirez any chance of getting some feedback on this?

[supercaracal]

I think we need more maintainers. #752

[ioquatix]

@supercaracal I am happy to maintain this if you add me to this repo.

[supercaracal]

I'm sorry, I don't have permission to add to the repo.

[ioquatix]

Who can help merge this?

[supercaracal]

@byroot @rafaelfranca Sorry to bother you when you're so busy. Would you have plans for increasing maintainers of this repository?

[mhenrixon]

I really need this!

[ioquatix]

@mperham any chance of getting someone to review/merge this? @mhenrixon want's to try it out in sidekiq.

[byroot]

Sorry to bother you when you're so busy. Would you have plans for increasing maintainers of this repository?

Even if we wanted we couldn't. We're not repository admin.

[ioquatix]

@byroot who is the admin? Can you make me a maintainer so I can maintain this implementation?

[ioquatix]

@mperham if we can't get this merged, is there documentation regarding what methods we need to implement so we can make a shim between https://github.com/socketry/async-redis and sidekiq?

[byroot]

who is the admin?

antirez / soveran / pietern / djanowski / badboy

Can you make me a maintainer so I can maintain this implementation?

No, only the owners above can.

Now that this is said, I understand that it's annoying not to get a timely review, nor much feedback at all. I've been swamped in the last month at work and got really no energy at all to review Redis PR.

I'm finally in vacation since Friday, so I do want to do a pass on open PRs soon™.

That being said I'd appreciate if you'd help me help you. You're not describing at all what the PR is doing, nor what async-io is. Which mean I have to guess myself by reading the thing and wonder if it's worth adding a support burden on redis-rb.

So yeah, I'm not the best maintainer, but I don't get the best PRs either...

[byroot]

The implementation seem decent beside a couple small nitpicks.

What I'm wondering is wether it should be in redis-rb itself. Others like em-synchrony are included in the core because it was done historically, but whenever I get an issue with them it's a PITA to fix them because they are both obscure, and I actually wonder if a significant amount of people still use it.

If it was me X years ago, I would rather have proposed to ship it in a 3rd party gem, and now I wonder the same for async-io.

[ioquatix]

@byroot Thanks for taking the time to review this PR.

That being said I'd appreciate if you'd help me help you. You're not describing at all what the PR is doing, nor what async-io is.

I'm happy to answer such a question. But this is the first time anyone asked such a question. All I did was make a PR and ensure all test pass. I welcome any and all feedback, but there was basically nothing for two months. So, I've just been waiting, until @mhenrixon told me they would find it useful, so I'm trying to get some more traction for this PR. I already tried contacting some admin but got no response. So, it's tricky. Is this project unmaintained? But, I see you made recent commits. Yes, for new contributor, it's not clear what next step is.

Now that this is said, I understand that it's annoying not to get a timely review, nor much feedback at all. I've been swamped in the last month at work and got really no energy at all to review Redis PR.

Just so you understand, I'm not really bothered by it. Sure, it's a bit frustrating since I invested some time... but I'm also maintainer and I understand the burden and don't want to pass it on to someone else. So please don't feel any stress on my part. If you don't have time for this, the best thing you can do is get others who can help involved.

Which mean I have to guess myself by reading the thing and wonder if it's worth adding a support burden on redis-rb.

Well, the only thing I can really do and as I've already said, is step in and help maintain it.

What I'm wondering is wether it should be in redis-rb itself

We already have it, https://github.com/socketry/async-redis, but sidekiq depends directly on redis-rb so some users don't have any choice. So, I tried to give them a choice.

I will answer your specific feedback in the appropriate threads. Hope this all comes across as cordial and supportive.

[byroot]

Is this project unmaintained?

Depends where you put the bar. I'm pretty much the only active contributor. I generally try to keep up with bug fixes & small features, but the last few months have been though. It doesn't help that Redis being used by a lot of people, we receive lots of "please fix my code for me" issues.

Hence why I'm on the fence about adding major new features. The gem already has quite a bit of tech debt, so I'm not very kin on merging stuff that would make the work harder in the future (NB: that doesn't mean I don't want to merge your PR, just that I'm on the fence).

We already have it, https://github.com/socketry/async-redis, but sidekiq depends directly on redis-rb so some users don't have any choice.

Not sure I understand that part. Sidekiq depends on redis-rb directly yes, but even if that driver is included in redis-rb users will still have to enable it manually. It's makes it a tiny bit easier, but not that much.

[ioquatix]

Fundamentally, I agree with you. I don't believe it's a sustainable model to have multiple implementations in a single gem/codebase (e.g. drivers).

That being said, the only reason why I made this PR in the way it is, is because I followed the existing code which uses different drivers, including em-synchrony. Originally, it was a POC. I wondered if async was mature enough to do this kind of thing. But when I found all tests passed, and async implementation was small, I thought it would be useful contribution. So, I'm not bothered about whether it gets merged or not. But we already have other users asking me, when will it get merged. So, that's why I think, maybe you should consider merging it. Because:

• It's smallest implementation of driver.
• It's desired by at least one other user, and maybe more.
• It's straight forward pure Ruby.
• I am happy to maintain it.

In any case, if you are active maintainer, and you can't get feedback from other maintainers/admin, well, either you can merge it, or you can ignore it. It doesn't bother me that much, but other users might be disappointed. I'm just happy that it worked :)

[byroot]

• It's smallest implementation of driver.

That's true. It really doesn't seem that bad of a support burden. But again, if anything I'd rather get rid of the other drivers than to add a new one.

It's desired by at least one other user, and maybe more.

Don't take it the wrong way, but as you certainly know as a maintainer, you can't (and shouldn't) support every single use case. You should aim to support what the vast majority needs, otherwise it never ends.

I am happy to maintain it.

That is a better argument. If I can ping you on async issues like I can ping @supercaracal on cluster issues, then yeah, it might make sense.

If you don't mind I'd like to sleep on it for a couple days. I'll fix focus on a bugfix release, and this might make it to a 4.2.0 (no promises).

[ioquatix]

Don't take it the wrong way, but as you certainly know as a maintainer, you can't (and shouldn't) support every single use case. You should aim to support what the vast majority needs, otherwise it never ends.

Yes, sure. The only reason why I mention this is because there is actually a real use case apparently. It's not just theoretical :)

this might make it to a 4.2.0 (no promises).

I'm sure some users would be very happy about that. On the other hand, as I said, it doesn't bother me that much. But whatever the decision, either merge or close this PR.

[ioquatix]

Any progress on this?

[byroot]

Right sorry. I decided not to merge it.

[ioquatix]

Okay, thanks for the update. Maybe you can update the README and add a link to https://github.com/socketry/async-redis/ for people who want an async-compatible Redis client.

[byroot]

Absolutely. Feel free to submit a PR with such README change.