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.
olix0r
left a comment
olix0r
left a comment
There was a problem hiding this comment.
If the Debug bounds are a problem, we could probably remove them?
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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. |
5 participants
Noteworthy changes:
newuses OS entropy,from_rngtakes aR: Rngand seeds the randomness from there.from_rngis fallible, since randomness generators can be fallible.BalanceLayerwas renamed toMakeBalanceLayer, since it is notreally a
BalanceLayer. The name ofBalanceMakewas also"normalized" to
MakeBalance.Another observation: the
Debugbound onLoad::Metricinp2c::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
MakeBalanceLayerorMakeBalance(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.