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

Support async command evaluation #917

Merged
merged 24 commits into from
Nov 17, 2023
Merged

Support async command evaluation #917

merged 24 commits into from
Nov 17, 2023

Conversation

mberndt123
Copy link
Contributor

@mberndt123 mberndt123 commented Nov 11, 2023

Resolves #916

Hey @mijicd,

I've hacked together something to implement the improved async support that I laid out in #916. The code could certainly be made a little bit prettier here and there, but the primary aim here is to try out the idea and see if you consider this a viable way forward.

The essential changes are:

  • change from requests.offer(…stuff…) *> promise.await to requests.offer(…stuff…).as(promise.await)), which makes it possible to continue processing as soon as the request was placed in the queue (SingleNodeExecutor.scala:35)
  • add a type parameter to the Redis trait to indicate whether it will return UIO[IO[RedisError, A]] or IO[RedisError, A] (Redis.scala:23)
  • change Redis.singleNode and Redis.local to provide both a synchronous and an asynchronous implementation of the Redis client (Redis.scala:45)

The resources like Queues and the Redis connection are shared between the synchronous and asynchronous Redis client, see the changes in SingleNodeExecutor.

The biggest drawback right now is that I haven't yet looked into the cluster support, so the cluster ZLayer doesn't give you an AsyncRedis object. I'd like to get some feedback on the general approach first before tackling that.

@CLAassistant
Copy link

CLAassistant commented Nov 11, 2023

CLA assistant check
All committers have signed the CLA.

@mberndt123 mberndt123 marked this pull request as ready for review November 15, 2023 16:57
@mberndt123 mberndt123 requested a review from a team as a code owner November 15, 2023 16:57
mijicd
mijicd previously approved these changes Nov 16, 2023
Copy link
Member

@mijicd mijicd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions/nitpicks, otherwise LGTM

modules/redis/src/main/scala/zio/redis/Redis.scala Outdated Show resolved Hide resolved
modules/redis/src/main/scala/zio/redis/GenRedis.scala Outdated Show resolved Hide resolved
modules/redis/src/main/scala/zio/redis/GenRedis.scala Outdated Show resolved Hide resolved
@mijicd mijicd changed the title Async support (for ticket #916) Support async command evaluation Nov 17, 2023
@mijicd mijicd merged commit bac8cc1 into zio:master Nov 17, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

better async/pipelining support?
3 participants