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

Make reaper workloads modifiable #985

Merged

Conversation

FinleyMcIlwaine
Copy link
Contributor

@FinleyMcIlwaine FinleyMcIlwaine commented Apr 26, 2024

It is convenient to be able to modify reaper workloads outside of the reaperAction in scenarios where we don't want to wait for reaperDelay for the jobs to be updated. This PR adds an atomic reaperModify function to the reaper API which enables this.

Since this changes the visible Reaper constructor, I did a major version bump (auto-update 0.1.6 -> 0.2.0) and added a changelog entry. I also bumped the warp dependency on auto-update upper bound.

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR

FinleyMcIlwaine added a commit to FinleyMcIlwaine/wai that referenced this pull request Apr 26, 2024
`cancel` used to mark the given `Handle` as `Canceled`, which relied on the
reaper to run and clean up the `Canceled` handles. This is not good for long
timeouts when many streams are opened, as a lot of `Canceled` handles build up
in the reaper's workload. This commit uses the reaper's new `reaperModify` API
to immediately remove the given `Handle` in `cancel`. This obsoletes the
`Canceled` constructor of `State`, so we remove that as well.

We update the auto-update, time-manager, and http2 dependencies of warp to
include the new changes as well.

The related auto-update changes are in PR yesodweb#985.
FinleyMcIlwaine added a commit to FinleyMcIlwaine/wai that referenced this pull request Apr 26, 2024
`cancel` used to mark the given `Handle` as `Canceled`, which relied on the
reaper to run and clean up the `Canceled` handles. This is not good for long
timeouts when many streams are opened, as a lot of `Canceled` handles build up
in the reaper's workload. This commit uses the reaper's new `reaperModify` API
to immediately remove the given `Handle` in `cancel`. This obsoletes the
`Canceled` constructor of `State`, so we remove that as well.

We update the auto-update, time-manager, and http2 dependencies of warp to
include the new changes as well.

The related auto-update changes are in PR yesodweb#985.
FinleyMcIlwaine added a commit to FinleyMcIlwaine/wai that referenced this pull request Apr 26, 2024
`cancel` used to mark the given `Handle` as `Canceled`, which relied on the
reaper to run and clean up the `Canceled` handles. This is not good for long
timeouts when many streams are opened, as a lot of `Canceled` handles build up
in the reaper's workload. This commit uses the reaper's new `reaperModify` API
to immediately remove the given `Handle` in `cancel`. This obsoletes the
`Canceled` constructor of `State`, so we remove that as well.

We update the auto-update and time-manager dependencies of warp to include the
new changes as well.

The related auto-update changes are in PR yesodweb#985.
FinleyMcIlwaine added a commit to FinleyMcIlwaine/wai that referenced this pull request Apr 26, 2024
`cancel` used to mark the given `Handle` as `Canceled`, which relied on the
reaper to run and clean up the `Canceled` handles. This is not good for long
timeouts when many streams are opened, as a lot of `Canceled` handles build up
in the reaper's workload. This commit uses the reaper's new `reaperModify` API
to immediately remove the given `Handle` in `cancel`. This obsoletes the
`Canceled` constructor of `State`, so we remove that as well.

We update the auto-update and time-manager dependencies of warp to include the
new changes as well.

The related auto-update changes are in PR yesodweb#985.
@FinleyMcIlwaine FinleyMcIlwaine force-pushed the finley/modifiable-workload branch 2 times, most recently from 43ca31c to 9893875 Compare April 26, 2024 22:54
FinleyMcIlwaine added a commit to FinleyMcIlwaine/wai that referenced this pull request Apr 26, 2024
`cancel` used to mark the given `Handle` as `Canceled`, which relied on the
reaper to run and clean up the `Canceled` handles. This is not good for long
timeouts when many streams are opened, as a lot of `Canceled` handles build up
in the reaper's workload. This commit uses the reaper's new `reaperModify` API
to immediately remove the given `Handle` in `cancel`. This obsoletes the
`Canceled` constructor of `State`, so we remove that as well.

We update the time-manager dependency of warp to include the new changes as
well.

The related auto-update changes are in PR yesodweb#985.
It is convenient to be able to modify reaper workloads outside of the
`reaperAction` in scenarios where we don't want to wait for `reaperDelay` for
the jobs to be updated. This PR adds an atomic `reaperModify` function to the
reaper API which enables this.

Since this changes the visible `Reaper` constructor, I did a major version bump
(`auto-update` 0.1.6 -> 0.2.0) and added a changelog entry. I also bumped the
`warp` dependency on `auto-update`.
FinleyMcIlwaine added a commit to FinleyMcIlwaine/wai that referenced this pull request Apr 26, 2024
`cancel` used to mark the given `Handle` as `Canceled`, which relied on the
reaper to run and clean up the `Canceled` handles. This is not good for long
timeouts when many streams are opened, as a lot of `Canceled` handles build up
in the reaper's workload. This commit uses the reaper's new `reaperModify` API
to immediately remove the given `Handle` in `cancel`. This obsoletes the
`Canceled` constructor of `State`, so we remove that as well.

We update the time-manager dependency of warp to include the new changes as
well.

The related auto-update changes are in PR yesodweb#985.
@kazu-yamamoto kazu-yamamoto self-requested a review April 27, 2024 05:54
@kazu-yamamoto
Copy link
Contributor

@snoyberg I'm OK with this PR.
But I don't remember why the Reaper constructor is exported.
Should we stop exporting it and export it from Reaper.Internal?

@kazu-yamamoto kazu-yamamoto merged commit efa8e3b into yesodweb:master Apr 27, 2024
@kazu-yamamoto
Copy link
Contributor

Merged anyway.
I will probably make Reaper.Internal later.
Thank you for your contribution!

kazu-yamamoto pushed a commit to kazu-yamamoto/wai that referenced this pull request Apr 27, 2024
`cancel` used to mark the given `Handle` as `Canceled`, which relied on the
reaper to run and clean up the `Canceled` handles. This is not good for long
timeouts when many streams are opened, as a lot of `Canceled` handles build up
in the reaper's workload. This commit uses the reaper's new `reaperModify` API
to immediately remove the given `Handle` in `cancel`. This obsoletes the
`Canceled` constructor of `State`, so we remove that as well.

We update the time-manager dependency of warp to include the new changes as
well.

The related auto-update changes are in PR yesodweb#985.
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