-
Notifications
You must be signed in to change notification settings - Fork 529
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
Changed tutorial so consumers are not overwhelmed by producers #3762
Conversation
```scala mdoc:compile-only | ||
import cats.effect._ | ||
import cats.effect.std.Console | ||
import cats.syntax.all._ | ||
import collection.immutable.Queue | ||
import scala.collection.immutable.Queue | ||
|
||
def producer[F[_]: Sync: Console](queueR: Ref[F, Queue[Int]], counter: Int): F[Unit] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sync
seems unnecessarily strong here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not strictly required, that is true, we could just use cats.Monad
. But I preferred to use CE type classes, as in the other code snippets that use Sync
and Async
. I'm ok with changing it but please let's be certain that is what we want 🙂 (implies changes in snippets, text and linked repo).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While a decision is taken about this I have addressed the other comments, thanks for the input!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely think Sync
/Async
is too much here. But since it's already there, I don't think there is any need to change them in this PR. It's fine to leave it for a followup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs some fairly trivial conflict resolution and I think it's ready to merge! Thank you!
The second section of the tutorial describes how to use fibers to solve the producer-consumer problem. Thing is, as the code is now, producers outpace consumers so much that consumers seem to be 'frozen'. The explication seems to be the
Ref.modify
run by consumers is way slower than theRef.getAndUpdate
call run by producers. This forces consumers to retry once and again to run themodify
call.This PR is to add text at the end of the first subsection (the one introducing a 'naive' solution to the producers-consumers problem) to describe the problem and to propose several solutions: using a
.sleep
call (the one used in the code samples); usingAtomicCell
instead ofRef
, or using a bounded queue (which is implemented in another section afterwards).The new version of the tutorial can be read online here. All code samples are available here.
Other minor changes:
transfer
andtransmit
functions in a single onetransfer
in order to simplify code and ease understandingSync[F].whenA
instead ofif ... else Sync[F].unit
catseffecttutorial.CopyFile
to correct onecatseffecttutorial.copyfile.CopyFile