DavidBM / rsmq-async-rs

RSMQ port to async rust. RSMQ is a simple redis queue system that works in any redis v2.6+
MIT License
43 stars 8 forks source link

impl std Error for RsmqError && use bb8_redis's connection pool to make Rsmq clonable #2

Closed GopherJ closed 3 years ago

GopherJ commented 3 years ago

This PR:

close: #3

GopherJ commented 3 years ago

@DavidBM Could you have a look at this PR? I would like to use Rsmq in multiple threads environment.

UPDATE: test seems failling, I'll dig into it later

DavidBM commented 3 years ago

Hi @GopherJ ! I'm checking it. It is quite big. I will create a new branch from this one and play a bit with it. I definitively like some of the changes.

GopherJ commented 3 years ago

@DavidBM Hi, thanks, it's not urgent because I can play with my branch. I just removed tests/support and added github actions. now the tests are passing on my pc.

Since I added db support, now every test is seperated into its own db, which avoids race

Feel free to give your opinions and changes that you may wanted

DavidBM commented 3 years ago

I will add some commits. I like the idea of having a docker action making test automatically, but I want the test to run locally too. The thing of the DBs doesn't seems very feasible, as when we have many test they will suffer of race conditions too. But honestly I don't know how to solve it if each test is not launching their own redis instance. I will try to see if we can mix both approaches.

DavidBM commented 3 years ago

For what I see bb8 is only Tokio based. So that would remove the capability of rsmq-async-rs to use async_std. I'm trying to find another solutions to this that doesn't tie the library to one executor.

GopherJ commented 3 years ago

@DavidBM I think we can use docker to run tests locally and CI services for running tests on cloud. Something like this: https://github.com/casbin-rs/diesel-adapter/blob/master/scripts/setup-db.sh

bb8 is tokio based, yes if you can find other libraries I can switch to it.

Or we can use synchronous thread pool like r2d2.

DavidBM commented 3 years ago

I'm thinking in splitting the method function from the connection state and adding another parameter to them for adding the connection. Then we can build some facade on top of them where the connection is build from different builders. I think that would work. We can have a trait that defined all the methods and then several implementation with several connection options.

DavidBM commented 3 years ago

My main concern about using one instance of redis (docker or other) are the race conditions that it may provoke. So, if possible, I would like to stick with a redis instance per test.

Btw, I didn't say it, but thank you so much! That is a very nice work. We just need to find the best way to integrate it :)

GopherJ commented 3 years ago

Yes maybe something like:

#[async_trait::async_trait]
trait ConnectionController {
   async fn get_conn(&self) -> &mut redis::aio::Connection;
}

impl<T> Rsmq<T> where T: ConnectionController {
 pub fn new_with_controller(controller: T)  -> Self<T> {}
}

I'm ok with it but this can may be done in another PR

DavidBM commented 3 years ago

Yeah. For me is important to try not breaking the API. So, I will try to merge everything together and have the features you implemented (because they are good)

DavidBM commented 3 years ago

Hi @GopherJ !

Check the branch now (last commit) and let me know if that helps you. I will check later the thing about having a CI on github.

GopherJ commented 3 years ago

@DavidBM Cool! I just had a look, it looks good to me! Thanks for your awesome work!

DavidBM commented 3 years ago

Thanks for the PR! It was very nice to add these features to the library