Implement transactions API by anovakovic01 · Pull Request #733 · zio/zio-redis

anovakovic01 · 2023-01-21T11:50:38Z

This PR resolves #156.

Summary:

Add support for MULTI-EXEC transactions,
Add compositional zipping of commands in a Redis transaction,
Add a transactional version of all commands,
Add a dedicated transactional connection pool.

mijicd · 2023-02-15T08:35:44Z

+      val lcount = left match {
+        case value: TransactionOutput[_, _, _] => value.count
+        case _                                 => 1
+      }
+
+      val rcount = right match {
+        case value: TransactionOutput[_, _, _] => value.count
+        case _                                 => 1
+      }


You can factor these two out, not much benefit from nesting them this way.

mijicd · 2023-02-15T08:36:21Z

+    val left: Output[A]
+    val right: Output[B]
+
+    def tryDecode(respValue: RespValue)(implicit codec: BinaryCodec): Out =


This should probably be final.

mijicd · 2023-02-15T08:36:28Z

+    val left: Output[A]
+    val right: Output[B]


These can be defs.

mijicd · 2023-02-15T08:36:55Z

+    def tryDecode(respValue: RespValue)(implicit codec: BinaryCodec): Out =
+      respValue match {
+        case RespValue.Array(values) =>
+          this match {


Let's introduce self (both for consistency and safety).

mijicd · 2023-02-15T08:37:42Z

+          this match {
+            case _: Zip[_, _] =>
+              (left, right) match {
+                case (l: TransactionOutput[_, _, _], r: TransactionOutput[_, _, _]) =>


Don't introduce names, shadow them instead - less concepts to deal with.

mijicd · 2023-02-15T08:39:10Z

+  final case class Zip[+A, +B](
+    left: Output[A],
+    right: Output[B]
+  ) extends TransactionOutput[A, B, (A, B)]
+
+  final case class ZipLeft[+A, +B](
+    left: Output[A],
+    right: Output[B]
+  ) extends TransactionOutput[A, B, A]
+
+  final case class ZipRight[+A, +B](
+    left: Output[A],
+    right: Output[B]
+  ) extends TransactionOutput[A, B, B]


Nit: I think you can write them down in a line-per-class manner:

final case class Zip[+A, +B](left: Output[A], right: Output[B]) extends TransactionOutput[A, B, (A, B)]

mijicd · 2023-02-15T08:40:04Z

+  def codec: BinaryCodec
+  def executor: RedisExecutor


Why do you need them to be public?

mijicd · 2023-02-15T08:40:41Z

+
+  def commit: ZIO[Redis, RedisError, Out] =
+    ZIO.serviceWithZIO[Redis] { redis =>
+      this match {


self (for reasons mentioned above)

mijicd · 2023-02-15T08:42:09Z

+import zio.schema.codec.BinaryCodec
+import zio.{URLayer, ZLayer}
+
+trait Redis extends api.Sets


This seems to duplicate the entire API, do we really need it? Can't we have transactional combinators on top of the existing API and use transactions as "scoping" mechanism?

mijicd · 2023-02-15T08:44:03Z

+                  (
+                    l.tryDecode(RespValue.Array(values.take(l.count))),
+                    r.tryDecode(RespValue.Array(values.drop(l.count).take(r.count)))
+                  ).asInstanceOf[Out]


We have some "gray zone" safety here.

Add transactional package. Add transactional sets API. Add few transactional sets tests. Signed-off-by: Aleksandar Novakovic <anovakovic01@gmail.com>

Extract all of the commands to the commands package in order to share them between transactional and normal API. Signed-off-by: Aleksandar Novakovic <anovakovic01@gmail.com>

Signed-off-by: Aleksandar Novakovic <anovakovic01@gmail.com>

mijicd · 2023-04-15T15:14:01Z

        }
      ),
-      suite("Stralgo")(
+      suite("StrAlgo")(


Let's do this in a separate PR.

mijicd · 2023-04-15T15:15:39Z

+  def zipLeft[A](that: RedisTransaction[A]): RedisTransaction[Out] =
+    ZipLeft(this, that)
+
+  def zipRight[A](that: RedisTransaction[A]): RedisTransaction[A] =
+    ZipRight(this, that)


mijicd · 2023-04-15T15:16:07Z

+sealed trait RedisTransaction[+Out] { self =>
+  import RedisTransaction._
+
+  def commit: ZIO[Redis, RedisError, Out] =


All methods should be final

mijicd · 2023-04-15T15:19:38Z

+private[redis] trait Cluster extends RedisEnvironment {
+  import Cluster._
+
+  final val _asking: RedisCommand[Unit, Unit] = askingCommand(codec, executor)


Two things:

let's not name things like this

are these exist to "help" with transactional package?

Generally speaking, they may be useful, but we don't have a "proof" for that (i.e. benchmarks), and they don't improve anything on schema-dependent commands.

These are used by both regular API and Transactional one. The idea was to define a command once and use it from both APIs. It was not introduced for optimization purposes. Also, I've done this because I don't know how to use the same API for both. The reason is the runtime behaviour. Regular API sends a command and receives a response, while the transactional one sends a command and has to remember what type of output it would have to parse later when EXEC is sent. That's why I had to introduce RedisTransaction which is basically transactional command.

mijicd · 2023-04-15T15:29:38Z

+        case Single(command, in) =>
+          command.run(in)


Is it possible reuse the existing API here (e.g. pass by name), rather than having a dedicated package full of duplicates?

mijicd · 2023-04-15T15:30:19Z

+import zio.redis.{Output, RedisCommand, RedisError}
+import zio.{ZIO, Zippable}
+
+sealed trait RedisTransaction[+Out] { self =>


I'd still prefer it in zio.redis rather than in a separate package.

mijicd · 2023-04-15T15:30:55Z

Offtopic: please sync with master or re-open as it's diverging quickly :)

anovakovic01 force-pushed the feature/transactions branch from 9c60cc1 to dfa7d75 Compare January 21, 2023 11:57

anovakovic01 force-pushed the feature/transactions branch from dfa7d75 to 7f0c7cf Compare February 5, 2023 12:27

anovakovic01 force-pushed the feature/transactions branch from 7f0c7cf to e1f0d29 Compare February 13, 2023 15:12

mijicd reviewed Feb 15, 2023

View reviewed changes

anovakovic01 force-pushed the feature/transactions branch from e1f0d29 to f5ce881 Compare February 21, 2023 12:17

anovakovic01 force-pushed the feature/transactions branch from f5ce881 to 7f5cbda Compare March 18, 2023 01:17

anovakovic01 added 3 commits March 20, 2023 18:48

Add initial Redis Transactions API

5a63f4c

Add transactional package. Add transactional sets API. Add few transactional sets tests. Signed-off-by: Aleksandar Novakovic <anovakovic01@gmail.com>

Extract all commands to the commands package

fa0616c

Extract all of the commands to the commands package in order to share them between transactional and normal API. Signed-off-by: Aleksandar Novakovic <anovakovic01@gmail.com>

Add zippable to the transactions API

c0fd6d1

Signed-off-by: Aleksandar Novakovic <anovakovic01@gmail.com>

anovakovic01 force-pushed the feature/transactions branch from e0a8ed5 to c0fd6d1 Compare March 20, 2023 18:00

anovakovic01 requested a review from jdegoes March 20, 2023 18:00

anovakovic01 added 2 commits March 20, 2023 19:56

Replace optional when with if else

d6c1591

Signed-off-by: Aleksandar Novakovic <anovakovic01@gmail.com>

Add map redis transaction encoding

6525ddf

Signed-off-by: Aleksandar Novakovic <anovakovic01@gmail.com>

mijicd reviewed Apr 15, 2023

View reviewed changes

guizmaii changed the base branch from master to main February 12, 2025 07:36

Conversation

anovakovic01 commented Jan 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mijicd commented Apr 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants