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

Bump CE and fs2 to the latest version #1246

Merged
merged 1 commit into from
Sep 27, 2023
Merged

Conversation

aartigao
Copy link
Contributor

Fixes #1234.

NOTE: I had to bump Scala 3 lib because since 3.8.0 fs2 uses 3.3.x

@aartigao
Copy link
Contributor Author

In addition to the changes suggested, while removing the TODO in produceRecord[F[_], K, V] method, I noticed the usage of Promise there inside Callback.

Consider the following code that uses directly the Producer API method with signature Future<RecordMetadata> send(ProducerRecord<K, V> record); and waits asynchronously to the result of the returned Future using F.async_ 👇🏽

private[kafka] def produceRecord[F[_], K, V](
  keySerializer: KeySerializer[F, K],
  valueSerializer: ValueSerializer[F, V],
  producer: KafkaByteProducer,
  blocking: Blocking[F]
)(
  implicit F: Async[F]
): ProducerRecord[K, V] => F[F[(ProducerRecord[K, V], RecordMetadata)]] =
  record =>
    asJavaRecord(keySerializer, valueSerializer, record).flatMap { javaRecord =>
      blocking {
        producer.send(javaRecord)
      }.map { future =>
        F.async_ { cb =>
          Try(future.get) match {
            case Success(metadata)               => cb(Right((record, metadata)))
            case Failure(ex: ExecutionException) => cb(Left(ex.getCause))
            case Failure(exception)              => cb(Left(exception))
          }
        }
      }
    }

According to async_ documentation:

This function can be thought of as a safer, lexically-constrained version of `Promise`,
where `IO` is like a safer, lazy version of `Future`.

What do you think about it? TBH I'm not really sure if there's any benefit on using async_ or if it's actually worse 🤷🏽 (the Try(future.get) part is the one that concerns me).

cc @vlovgr

@vlovgr
Copy link
Contributor

vlovgr commented Sep 26, 2023

What do you think about it? TBH I'm not really sure if there's any benefit on using async_ or if it's actually worse 🤷🏽 (the Try(future.get) part is the one that concerns me).

@aartigao With the new cancellation semantics in cats-effect 3.5.0, F.async_ is uncancelable. Try(future.get) is also blocking, so I'd suggest we keep what you already have in this pull request. 👍

@aartigao aartigao merged commit 7b230df into fd4s:series/3.x Sep 27, 2023
7 checks passed
@aartigao aartigao deleted the bump-ce-fs2 branch September 27, 2023 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

upgrade 3.x to CE 3.5.1 / fs2 3.8.0
2 participants