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 zio2.x #598

Merged
merged 42 commits into from
Aug 24, 2022
Merged

Support zio2.x #598

merged 42 commits into from
Aug 24, 2022

Conversation

jxnu-liguobin
Copy link
Collaborator

@jxnu-liguobin jxnu-liguobin commented May 26, 2022

closed #462 Initially commit to upgrade zio2.x

Blocking until ZIO 2.0.0

@jxnu-liguobin jxnu-liguobin changed the title closed #462 Initially commit to upgrading zio2.x, need to get help in… upgrading zio2.x, need to get help in… May 26, 2022
@jxnu-liguobin jxnu-liguobin changed the title upgrading zio2.x, need to get help in… Support zio2.x May 26, 2022
@@ -152,8 +151,9 @@ object Output {
final case class OptionalOutput[+A](output: Output[A]) extends Output[Option[A]] {
protected def tryDecode(respValue: RespValue)(implicit codec: Codec): Option[A] =
respValue match {
case RespValue.NullBulkString | RespValue.NullArray | RespValue.BulkString(Chunk.empty) => None
case other => Some(output.tryDecode(other))
case RespValue.NullBulkString | RespValue.NullArray => None
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FIXME

Copy link
Member

Choose a reason for hiding this comment

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

Please elaborate.

Copy link
Collaborator Author

@jxnu-liguobin jxnu-liguobin May 27, 2022

Choose a reason for hiding this comment

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

Compile failed

stable identifier required, but zio.Chunk.empty found.
        case RespValue.NullBulkString | RespValue.NullArray | RespValue.BulkString(Chunk.empty)      => None

because in zio2.x Chunk.empty is def?

… into series/2.x

� Conflicts:
�	redis/src/main/scala/zio/redis/TestExecutor.scala
@jxnu-liguobin jxnu-liguobin marked this pull request as ready for review August 15, 2022 14:00
@jxnu-liguobin jxnu-liguobin requested a review from a team as a code owner August 15, 2022 14:00
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.

First of all, thank you for taking care of this. Since it is a huge change we may need to do a few review passes, but it looks good in general.

P.S. It looks like we're going to "forget" about 1.x compatible release (which is not necessary bad considering our timing). That said, please resolve the conflicts.


import java.util.concurrent.TimeUnit

@State(Scope.Thread)
@State(org.openjdk.jmh.annotations.Scope.Thread)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary? The import is there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Scope conflicts with zio.Scope, I have imported zio.Scope to hide

example/src/main/scala/example/ContributorsCache.scala Outdated Show resolved Hide resolved
example/src/main/scala/example/Main.scala Outdated Show resolved Hide resolved
import zio.redis.RedisConfig

final case class AppConfig(redis: RedisConfig, server: ServerConfig)

object AppConfig {
val descriptor: ConfigDescriptor[AppConfig] = DeriveConfigDescriptor.descriptor[AppConfig]
val confDescriptor: _root_.zio.config.ConfigDescriptor[AppConfig] = descriptor[AppConfig]
Copy link
Member

Choose a reason for hiding this comment

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

What happened here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

descriptor is now the top-level method instead of DeriveConfigDescriptor's method. And i will delete package prefix

package object example {
type ContributorsCache = Has[ContributorsCache.Service]
type ContributorsCache = ContributorsCache.Service
Copy link
Member

Choose a reason for hiding this comment

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

I think we should get rid of old module pattern encoding as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will use case class

@@ -28,57 +27,59 @@ import java.nio.channels.{AsynchronousSocketChannel, Channel, CompletionHandler}
private[redis] object ByteStream {
trait Service {
def read: Stream[IOException, Byte]
def write(chunk: Chunk[Byte]): IO[IOException, Unit]
def write(chunk: Chunk[Byte]): IO[IOException, Option[Unit]]
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of option?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since Option is required for matching here(case State.Start), this is one of the areas I find most confusing.

  private[redis] final val Decoder: ZPipeline[Any, RedisError.ProtocolError, Byte, Option[RespValue]] = {
    val Sinker: ZSink[Any, RedisError.ProtocolError, String, String, Option[RespValue]] = {
      import internal.State
      ZSink.fold[String, State](State.Start)(_.inProgress)(_ feed _).mapZIO {
        case State.Done(value) => ZIO.succeedNow(Some(value))
        case State.Failed      => ZIO.fail(RedisError.ProtocolError("Invalid data received."))
        // ZSink fold will return a State.Start when contFn is false
        case State.Start => ZIO.succeedNow(None)
        case other       => ZIO.dieMessage(s"Deserialization bug, should not get $other")
      }

    }

lazy val live: URLayer[Has[RedisExecutor] with Has[Codec], Has[Redis]] =
ZLayer.identity[Has[Codec]] ++ ZLayer.identity[Has[RedisExecutor]] >>> RedisService
lazy val live: URLayer[RedisExecutor with Codec, Redis] =
ZLayer.environment[Codec] ++ ZLayer.environment[RedisExecutor] >>> RedisService
Copy link
Member

Choose a reason for hiding this comment

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

I'd get rid of operators.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean i should use ZLayer.make?

redis/src/main/scala/zio/redis/RedisExecutor.scala Outdated Show resolved Hide resolved
private[redis] final val Decoder: Transducer[RedisError.ProtocolError, Byte, RespValue] = {
import internal.State
private[redis] final val Decoder: ZPipeline[Any, RedisError.ProtocolError, Byte, Option[RespValue]] = {
val Sinker: ZSink[Any, RedisError.ProtocolError, String, String, Option[RespValue]] = {
Copy link
Member

Choose a reason for hiding this comment

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

Let's drop the type, and use the lower camel case.

Copy link
Member

Choose a reason for hiding this comment

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

Please don't mark unresolved issues as resolved.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, sorry, I thought it was just the outermost type declaration that should be removed.

@jxnu-liguobin jxnu-liguobin requested a review from mijicd August 22, 2022 02:19
Comment on lines 37 to 40
case class ServiceLive(r: Redis, s: Sttp) extends Service {
override def fetchAll(repository: Repository): IO[ApiError, Contributors] =
(read(repository) <> retrieve(repository)).provide(ZLayer.succeed(r), ZLayer.succeed(s))
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's abandon the old encoding, e.g.:

trait ContributorsCache {
  def fetchAll(...)
}

final case class ContributorsCacheLive(...) extends ContributorsCache {
...
}

Also, let's please adhere to the multi-unit files naming convention.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed

(read(repository) <> retrieve(repository)).provide(ZLayer.succeed(r), ZLayer.succeed(s))
}

object ServiceLive {
Copy link
Member

Choose a reason for hiding this comment

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

Please adjust accordingly.

}

object ServiceLive {
lazy val layer: ZLayer[Redis with Sttp, Nothing, Service] = ZLayer.fromZIO {
Copy link
Member

Choose a reason for hiding this comment

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

You don't need fromZIO, apply works as well. Also, you can use URLayer.

get(repository.key)
.returning[String]
.someOrFail(ApiError.CacheMiss(repository.key))
.map(_.fromJson[Contributors])
.rightOrFail(ApiError.CorruptedData)
.refineToOrDie[ApiError]
.foldZIO(_ => ZIO.fail(ApiError.CorruptedData), s => ZIO.succeed(s.getOrElse(Contributors(Chunk.empty))))
Copy link
Member

Choose a reason for hiding this comment

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

What happened here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I found no rightOrFail method, so I changed it here.

@@ -22,41 +22,38 @@ import example.config.{AppConfig, ServerConfig}
import sttp.client3.asynchttpclient.zio.AsyncHttpClientZioBackend
import zhttp.service.server.ServerChannelFactory
import zhttp.service.{EventLoopGroup, Server}
import zio.Console.printLine
Copy link
Member

Choose a reason for hiding this comment

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

I would not import it. Console.printLine reads nicer.

import zio.redis.RedisError.ProtocolError
import zio.redis.RespValue.{BulkString, bulkString}
import zio.redis.TestExecutor.{KeyInfo, KeyType}
import zio.stm.{random => _, _}
import zio.stm._
import zio.{Clock, Random, _}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import zio.{Clock, Random, _}
import zio._

import zio.redis.Input._
import zio.redis.Output._
import zio.redis.ResultBuilder._
import zio.redis._
import zio.schema.Schema
import zio.{Chunk, Has, ZIO}
import zio.{Chunk, ZIO, _}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import zio.{Chunk, ZIO, _}
import zio._

import zio.redis.Input._
import zio.redis.Output._
import zio.redis.ResultBuilder._
import zio.redis._
import zio.schema.Schema
import zio.{Chunk, Has, ZIO}
import zio.{Chunk, ZIO, _}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import zio.{Chunk, ZIO, _}
import zio._

import zio.redis.Input._
import zio.redis.Output._
import zio.redis.ResultBuilder._
import zio.redis._
import zio.schema.Schema
import zio.{Chunk, Has, ZIO}
import zio.{Chunk, ZIO, _}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import zio.{Chunk, ZIO, _}
import zio._

@jxnu-liguobin this pattern repeats a lot, please adjust it everywhere according to the suggestion.

private val TestLayer =
RedisExecutor.test ++ ZLayer.succeed(codec) >>> Redis.live
private val TestLayer = {
val redis = RedisExecutor.test ++ ZLayer.succeed(codec) >>> Redis.live
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to just use make.

@mijicd mijicd merged commit 06a2668 into zio:master Aug 24, 2022
@mijicd mijicd mentioned this pull request Aug 24, 2022
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.

Upgrade to ZIO 2.0
2 participants