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

hedge: use auto-resizing histograms #484

Merged
merged 1 commit into from
Nov 19, 2020
Merged

Conversation

hdevalence
Copy link
Contributor

The previous code used a fixed-size histogram with an upper bound of 10_000 ms
(10s). This meant that the Hedge middleware would display errors when used
with services that take longer than 10s to complete a response. Instead, use a
constructor that produces an auto-resizing histogram. In the future, if the
auto-resizing behavior is an issue, Tower could add a second constructor for
the Hedge middleware that allows specifying bounds, but for now this change is
transparent and avoids spurious errors.

@hdevalence
Copy link
Contributor Author

Closes #475

The previous code used a fixed-size histogram with an upper bound of 10_000 ms
(10s).  This meant that the `Hedge` middleware would display errors when used
with services that take longer than 10s to complete a response.  Instead, use a
constructor that produces an auto-resizing histogram.  In the future, if the
auto-resizing behavior is an issue, Tower could add a second constructor for
the Hedge middleware that allows specifying bounds, but for now this change is
transparent and avoids spurious errors.
@hawkw
Copy link
Member

hawkw commented Nov 19, 2020

the CI failure is spurious (looks like a tool could not be downloaded). i'll restart CI.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems good to me!

tower/src/hedge/rotating_histogram.rs Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Nov 19, 2020

Ah, hmm, looks like the cargo-hack CI job just always fails on PRs from forks? We should fix that. I'll just go ahead and merge this, though.

@hawkw hawkw merged commit d4d1c67 into tower-rs:master Nov 19, 2020
@hdevalence hdevalence deleted the hedge-resize branch November 19, 2020 22:29
hdevalence added a commit to ZcashFoundation/zebra that referenced this pull request Nov 19, 2020
hdevalence added a commit to ZcashFoundation/zebra that referenced this pull request Nov 20, 2020
teor2345 added a commit to teor2345/zebra that referenced this pull request Feb 17, 2021
When other tower-batch tasks drop, wake any tasks that are waiting for
a semaphore permit. Otherwise, tower-batch can hang.

We currently pin tower in our workspace to:
d4d1c67 hedge: use auto-resizing histograms (tower-rs/tower#484)

Copy tower/src/semaphore.rs from that commit, to pick up
tower-rs/tower#480.
dconnolly pushed a commit to ZcashFoundation/zebra that referenced this pull request Feb 17, 2021
When other tower-batch tasks drop, wake any tasks that are waiting for
a semaphore permit. Otherwise, tower-batch can hang.

We currently pin tower in our workspace to:
d4d1c67 hedge: use auto-resizing histograms (tower-rs/tower#484)

Copy tower/src/semaphore.rs from that commit, to pick up
tower-rs/tower#480.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants