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

Changed tutorial so consumers are not overwhelmed by producers #3762

Merged
merged 3 commits into from
Nov 26, 2023

Conversation

lrodero
Copy link
Contributor

@lrodero lrodero commented Jul 26, 2023

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 the Ref.getAndUpdate call run by producers. This forces consumers to retry once and again to run the modify 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); using AtomicCell instead of Ref, 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:

  • Typo as in PR fix typo in tutorial #3760
  • In copy file section merge transfer and transmit functions in a single one transfer in order to simplify code and ease understanding
  • In code samples use Sync[F].whenA instead of if ... else Sync[F].unit
  • Updated sbt version (1.9.3)
  • Changed wrong path catseffecttutorial.CopyFile to correct one catseffecttutorial.copyfile.CopyFile

@armanbilge armanbilge closed this Aug 6, 2023
@armanbilge armanbilge reopened this Aug 6, 2023
```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] =
Copy link
Contributor

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.

Copy link
Contributor Author

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).

Copy link
Contributor Author

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!

Copy link
Contributor

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.

docs/tutorial.md Show resolved Hide resolved
docs/tutorial.md Show resolved Hide resolved
docs/tutorial.md Outdated Show resolved Hide resolved
docs/tutorial.md Outdated Show resolved Hide resolved
docs/tutorial.md Show resolved Hide resolved
docs/tutorial.md Show resolved Hide resolved
docs/tutorial.md Show resolved Hide resolved
docs/tutorial.md Outdated Show resolved Hide resolved
docs/tutorial.md Outdated Show resolved Hide resolved
Copy link
Member

@djspiewak djspiewak left a 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!

@djspiewak djspiewak merged commit 328f61d into typelevel:series/3.x Nov 26, 2023
32 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants