Backpressure Interface for CE3 #1817
Conversation
| */ | ||
| def apply[F[_]]( | ||
| strategy: Strategy, | ||
| bound: Int |
There was a problem hiding this comment.
Since this is passed to the underlying semaphore, which accepts Long arguments, should we maybe have this be a Long too?
There was a problem hiding this comment.
Potentially, though the CE2 version uses an Int. Also, Long would be quite a large bound for what Backpressure is; Long.MaxValue would be a heck of a lot of concurrent effects 😅
There was a problem hiding this comment.
I agree. I was just raising a question on the API design. I'm fine either way.
|
Any objections here? I'd be happy to see this merged. |
| strategy: Strategy, | ||
| bound: Int | ||
| )(implicit GC: GenConcurrent[F, Throwable]): F[Backpressure[F]] = { | ||
| require(bound > 0) |
There was a problem hiding this comment.
Would it be better to raise than throw here? I'm surprised to find require in the code in a couple places, so there is precedent for this.
There was a problem hiding this comment.
Yeah... these instances exist because of the GenConcurrent constraint...
| .tryAcquire | ||
| .bracket { | ||
| case true => f.map(_.some) | ||
| case false => none[A].pure[F] |
There was a problem hiding this comment.
Could cache this value so it doesn't need to be recreated on every backpressured call, at the expense of always creating it once.
There was a problem hiding this comment.
Cool!! Can we add syntax for this? Like I just proposed for Supervisor in #2068.
|
@armanbilge if you're up for it, feel free to make the PR :) I'll merge for now to avoid delaying further (we're already past 3.2.0 though) |
|
@djspiewak I guess the we're shipping this? |
Created per the discussion on #1695 (comment)