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

Delete tasks endpoint #260

Merged
merged 6 commits into from
Jul 1, 2021
Merged

Delete tasks endpoint #260

merged 6 commits into from
Jul 1, 2021

Conversation

hannahhoward
Copy link
Contributor

@hannahhoward hannahhoward commented Jun 29, 2021

Goals

Be able to cancel either task schedules (not the running instances of scheduled tasks) or unstarted tasks

Implementation

This turned out to be enjoyable difficult, as SQLite apparently does not implement ALTER TABLE ADD/DROP CONSTRAINT. This led after much discussion to the idea to drop SQLite.

Here is the approach used to defaulting to PG:

  • most of us are never running integration tests locally any more, but if we do, everyone can handle their own setup
  • what we are doing frequently is writing unit tests
  • unit tests are an interesting problem cause not only do they need a database but it needs to be reset completely between tests
  • previously, we did this my just making a new temp directory and SQLite file
  • now, upon test start, we check if we can connect to a running PG instance on a host machine (if the dev is running PG locally, they can set env vars so that it connects)
  • if there is no host machine instance, start a temporary docker container for postgres. Connect there and tear it down on test terminate.
  • for resetting, since it's very time consuming to actually try to tear down and restart docker instances, we instead just reset all records in the db using truncate, a pretty standard practice for resetting databases for unit tests (that talk to the DB of course)

Otherwise, the implementation is pretty straightforward.

  • SQL queries
  • State method
  • Controller endpoint
  • Controller client method

add delete method to state. Unfortunately limitations of SQLite forced a division of migrations to
alter a constraint.
Add controller endpoint to access delete function on state
@willscott
Copy link
Collaborator

lets get rid of the sqlite support rather than adding this much complexity to support it

Copy link
Collaborator

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

A few suggestions, but looks good.

@@ -1078,3 +1083,29 @@ func (s *stateDB) ResetWorkerTasks(ctx context.Context, worker string) error {
}
return nil
}

func (s *stateDB) Delete(ctx context.Context, uuid string) error {
task, _, err := s.getWithTag(ctx, uuid)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be better to create a version of this function that is not transaction-wrapped and call to get and delete within the same transaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree.

transaction wrapping should happen in the calling code for a utility function like this, IMHO.

`

deleteTaskStatusLedgerSQL = `
DELETE FROM task_status_ledger WHERE uuid = $1
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might make sense to combine these into a single query:

deleteTaskAndStatueSQL = `
    DELETE FROM tasks WHERE uuid = $1;
    DELETE FROM task_status_ledger WHERE uuid = $1
`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh did I not push the fix? you actually can great ride of this with ON DELETE CASCADE

@@ -49,8 +49,8 @@ Dealbot Controller
| graphql | `DEALBOT_GRAPHQL_LISTEN` | exposed `host:port` for external public graphql queries |
| metrics | `DEALBOT_METRICS` | either `prometheus` to expose a `/metrics` api, or `log` to write metrics to stdout |
| identity | `DEALBOT_IDENTITY_KEYPAIR` | filepath of a libp2p identity to sign public records of dealbot activity |
| driver | `DEALBOT_PERSISTENCE_DRIVER` | `sqlite` or `postgres` |
| dbloc | `DEALBOT_PERSISTENCE_CONN` | the file (for sqlite) or db conn string from postgres |
| driver | `DEALBOT_PERSISTENCE_DRIVER` | `postgres` |
Copy link
Collaborator

Choose a reason for hiding this comment

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

get rid of entirely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that was what we decided?

Copy link
Collaborator

Choose a reason for hiding this comment

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

but now that there's only one entry we can get rid of the flag entirely?

@willscott willscott merged commit 564a5fd into main Jul 1, 2021
@willscott willscott deleted the feat/delete branch July 1, 2021 01:49
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