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

Task expiration and deletion #981

Merged
merged 6 commits into from
Apr 19, 2024
Merged

Task expiration and deletion #981

merged 6 commits into from
Apr 19, 2024

Conversation

inahga
Copy link
Contributor

@inahga inahga commented Apr 15, 2024

Supports #862.

Two methods are provided.

  • PATCH {task_id} with one field expiration that allows free manipulation of a task's expiration date. As alluded to in Change default task expiration to None #980, there is no semantic validation of the new expiration--this can be set to whatever the user would like. Changes are propagated to the aggregators.
  • DELETE {task_id} which sets the aggregators' expiration, if not already set and expired, and then tombstones the divviup-api view of the task by setting deleted_at. Currently, there is no effect of this flag--all other methods will ignore the deleted_at field.

Note that I'm assuming aggregators need to have support for the missing endpoints, otherwise we explicitly fail. I landed on this because we don't want to 'lie' to the user about deletion/expiration, since active aggregator-side tasks can still process data.

There is still more work to be done, but I'm keeping this PR from getting too long.

  • Other existing task endpoints are not aware of the deleted_at field, e.g. listing tasks still presents deleted tasks.
  • Need client/CLI support outside of basic definitions.
  • Need UI support.

src/entity.rs Fixed Show fixed Hide fixed
src/entity.rs Fixed Show fixed Hide fixed
src/entity/task/update_task.rs Fixed Show fixed Hide fixed
src/entity.rs Fixed Show fixed Hide fixed
src/entity.rs Fixed Show fixed Hide fixed
tests/integration/tasks.rs Fixed Show fixed Hide fixed
src/entity/task/update_task.rs Fixed Show fixed Hide fixed
@inahga inahga force-pushed the inahga/expire-tasks branch 3 times, most recently from b8bf913 to 5d72be8 Compare April 16, 2024 17:40
@inahga inahga marked this pull request as ready for review April 16, 2024 18:21
@inahga inahga requested a review from a team as a code owner April 16, 2024 18:21
@inahga
Copy link
Contributor Author

inahga commented Apr 17, 2024

Note that I'm assuming aggregators need to have support for the missing endpoints, otherwise we explicitly fail. I landed on this because we don't want to 'lie' to the user about deletion/expiration, since active aggregator-side tasks can still process data.

Hmm. It occurs to me that if the helper aggregator no longer exists, e.g. because the user is turning down their own BYOH, then tasks involving the deleted aggregator will be undeletable.

I suppose I should make it such that the leader aggregator must succeed in setting expiration, while the helper can be permissive of failure. This should be fine--the leader is what drives the protocol and is what incurs most of the cost.

src/entity/task/model.rs Outdated Show resolved Hide resolved
inahga and others added 4 commits April 17, 2024 18:11
* Exclude deleted tasks from task lists

* Client and CLI support for task expiry and deletion
@inahga inahga enabled auto-merge (squash) April 19, 2024 17:32
@inahga inahga merged commit 400e4c8 into main Apr 19, 2024
6 checks passed
@inahga inahga deleted the inahga/expire-tasks branch April 19, 2024 17:35
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.

3 participants