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

Promises & futures for Predis pipelines #643

Closed
jimbonator opened this issue Aug 19, 2020 · 6 comments
Closed

Promises & futures for Predis pipelines #643

jimbonator opened this issue Aug 19, 2020 · 6 comments
Labels
Milestone

Comments

@jimbonator
Copy link

One technique we've used with some success involves a miniature library I developed on top of Predis called Deferred. I demonstrated this library at RedisConf 2018.

Deferred formalizes a promises/futures model that use Predis pipelines (transaction, pipeline, and atomic pipeline) under-the-covers. It's a more object-oriented approach to pipelining, and improves encapsulation both for assembling commands and processing the responses.

In particular, it allows for responses to be bound directly to PHP variables, to automatically transform values and merge them into complex data structures, and provides notifications for logging/statistics.

My proposal is for this library (or its approach) to became a standard part of Predis, since I think there's a lot of common ground between the two. I'd be happy to help transition the existing code to Predis, or to assist in building a sub-module that follows Deferred's model.

More information can be found in Deferred's README.

@nrk
Copy link
Contributor

nrk commented Sep 7, 2020

I'm finally testing with actual code a new approach for the pipeline abstraction's internals in v2.0 using generators (yay for dropping old PHP versions!) and in my various tests to verify if the new design is actually practical and consistent (while giving more flexibility than before) I also tried to emulate the basic idea behind Deferred and in the end I think, unless I missed something along the way, that something like Deferred could be indeed plugged into the new design easily by extending the base Pipeline class (yeah we'll have just on class) and override 2 or 3 methods. I say "emulate" because I did not use promises / futures to keep my test simple, don't have time ATM, I'll try in the next few days or maybe I'll leave it up to you as soon as I'll push a draft PR 😉

@nrk nrk added this to the v2.0.0 milestone Sep 9, 2020
@nrk
Copy link
Contributor

nrk commented Sep 9, 2020

Alright I'm tentatively adding this feature for v2.0, at least for pipelines. To be honest I can't say for plain transactions yet, ideally Predis\Transaction\MultiExec should be re-implemented from the ground up based on an internal design similar to what I envisioned for pipelines, that's what I was planning to do anyway so we'll see.

For now I'm targeting a more lightweight approach compared to Deferred (no reduce(), no raw values, ...) but binding, listeners and trasformers are supported. We will have a Predis\Pipeline\DeferredPipeline returning Predis\Response\FutureResponse instances. DeferredPipeline::execute() will not return anything, values read from the pipeline will be accessible only by FutureResponse. Ideally DeferredPipeline should be easily extensible by third parties so that they can plug whatever response wrapper or logic to populate values. It should be possible to rewrite the full-fledged Deferred library on top of this.

My proposed implementation will follow in a few days.

@nrk
Copy link
Contributor

nrk commented Sep 14, 2020

Just a heads-up: I'm late on my plans as I was busy last week and it will take a few more days, nevertheless while I'm adding the last touches and polishing code for pipelines (almost ready to push a draft PR but still missing tests as they will require a ton of time and work) I started experimenting with transactions and at this point I think it will be indeed possible to share the same approach so we will probably have a Predis\Transaction\DeferredMultiExec returning Predis\Response\FutureResponse instances.

By the way Predis\Transaction\MultiExec will reuse a new underlying component created for pipelines meaning that transactions will be pipelined internally (outside of check-and-set operations, obviously) just like it happens with atomic pipelines. Having two separate abstractions still makes sense to me as they have a few key differences.

@nrk
Copy link
Contributor

nrk commented Sep 21, 2020

The new pipeline abstraction is live on #663.

I'm going to post a gist with my proof of concept for Predis\Pipeline\DeferredPipeline tomorrow (an actual and more curated PR will follow when #663 will be considered stable enough in terms of both code and internal design) but at least we can start playing with stuff and see how far we can take this.

I still have to find some common grounds between pipelines and transactions to share some exceptions, especially in case of aborted transactions, and make both truly interchangeable (as we discussed in other issues/pr) but I guess this will be possible only once we actually start re-implementing transactions (I have a local branch but it's a mess and incomplete, so it doesn't count yet).

@nrk
Copy link
Contributor

nrk commented Sep 22, 2020

Just posted my proof of concept on this gist, see also the included README file. It is far from being definitive but it's a starting point at least, I avoided a full-fledged repository because I just wanted to share the code and give everyone an idea of what I'm planning, ideally this will become a proper PR in the future. I'll keep the gist updated if breaking changes are pushed on the development branch tracked by #663.

@jimbonator
Copy link
Author

Apologies for not getting a chance to study your gist sooner. I'm happy to see most of the features I implemented in Deferred are present in your proof-of-concept.

From my casual glance-over, I'm unsure why asArray() and asGenerator() require special treatment. Couldn't they simply register themselves as transformers directly, rather than setting internal flags and adding them during finalize? Or, are you attempting to enforce a transformer ordering (where they always go last)? Just thinking of simplicity here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants