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
Spring cleaning for tower::balance #449
Conversation
Noteworthy changes: - All constructors now follow the same pattern: `new` uses OS entropy, `from_rng` takes a `R: Rng` and seeds the randomness from there. `from_rng` is fallible, since randomness generators can be fallible. - `BalanceLayer` was renamed to `MakeBalanceLayer`, since it is not _really_ a `BalanceLayer`. The name of `BalanceMake` was also "normalized" to `MakeBalance`. Another observation: the `Debug` bound on `Load::Metric` in `p2c::Balance`, while not particularly onerous, generates really confusing errors if you forget it include it. And crucially, the error never points at `Debug` (should we file a compiler issue?), so I pretty much had to guess my way to that being wrong in the doc example. It would probably be useful to add a documentation example to `MakeBalanceLayer` or `MakeBalance` (I suspect just one of them is fine, since they're basically the same). Since I've never used it, and find it hard to think of uses for it, it might be good if someone with more experience with it wrote one.
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.
If the Debug bounds are a problem, we could probably remove them?
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.
Looks good to me overall, thanks for working on improving the docs!
I had some notes, but none of them are really blockers.
/// Note that `Balance` requires that the `Discover` you use is `Unpin` in order to implement | ||
/// `Service`. This is because it needs to be accessed from `Service::poll_ready`, which takes | ||
/// `&mut self`. You can achieve this easily by wrapping your `Discover` in [`Box::pin`] before you | ||
/// construct the `Balance` instance. For more details, see [#319]. |
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.
probably best saved for a follow-up PR, but should Discover
just have an Unpin
bound on it? Since the unpin bound is only on the Service
impl for Balance
, this will currently fail very late, when a user tries to wrap a Balance
in another layer or actually call a Balance
, and it probably won't be super clear why Service
isn't implemented. If Discover
required Self: Unpin
, the compiler error would occur when the user writes a bad Discover
impl, which would be much clearer.
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 think we should avoid Unpin
bounds wherever possible. They are really sad, especially when we one day get generators that implement Stream
. This also ties into the larger discussion in #319.
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.
Hmm, but if Discover
is going to be polled in poll_ready
, it will always have to be Unpin
unless poll_ready
is changed to take a Pin<&mut Self>
receiver. In that case, we would be making a breaking change anyway and could remove an Unpin
bound from Discover
.
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 don't know if Discover
will only ever be polled in poll_ready
though? But you're right that if poll_ready
later changes to be Pin
, we could remove the bound then (although I think removing the bound is backwards-compatible anyway?).
@olix0r The bound is there just for tracing really, so that we can print the actual measured load. And I think that's a pretty valuable purpose. Really not sure what to do about that. |
Noteworthy changes:
new
uses OS entropy,from_rng
takes aR: Rng
and seeds the randomness from there.from_rng
is fallible, since randomness generators can be fallible.BalanceLayer
was renamed toMakeBalanceLayer
, since it is notreally a
BalanceLayer
. The name ofBalanceMake
was also"normalized" to
MakeBalance
.Another observation: the
Debug
bound onLoad::Metric
inp2c::Balance
, while not particularly onerous, generates reallyconfusing errors if you forget it include it. And crucially, the error
never points at
Debug
(should we file a compiler issue?), so I prettymuch had to guess my way to that being wrong in the doc example.
It would probably be useful to add a documentation example to
MakeBalanceLayer
orMakeBalance
(I suspect just one of them is fine,since they're basically the same). Since I've never used it, and find it
hard to think of uses for it, it might be good if someone with more
experience with it wrote one.