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

#[swirl::background_job] assumes serde::{Deserialize, Serialize} available #9

Closed
sgrif opened this issue Mar 18, 2019 · 1 comment · Fixed by #12
Closed

#[swirl::background_job] assumes serde::{Deserialize, Serialize} available #9

sgrif opened this issue Mar 18, 2019 · 1 comment · Fixed by #12

Comments

@sgrif
Copy link
Owner

sgrif commented Mar 18, 2019

Right now we require that the crate using this derive has serde = { version = "1.0", features = ["derive"] }. We should work without that requirement (either because they don't use serde, or they're doing the older #[macro_use] extern crate serde_derive;)

sgrif added a commit to sgrif/crates.io that referenced this issue Mar 18, 2019
New features in this version:

- Codegen for jobs which reduces boilerplate when defining them
- Jobs no longer need to be registered with the runner
- Rework of how `run_all_pending_jobs` works so errors loading the job
  actually bubble up
- Configurable timeout for how long to wait for a job to begin running

We aren't fully taking advantage of the timeout yet, I'd eventually like
to try rebuilding the runner in-process a few times when an error
happens, and then crash the process if it fails a few times in a row.
This needs a bit more refactoring though (I'm not sure if we want to
keep the `Repository` in memory like we are now, and whether we want to
assume a full re-clone is needed on error). For now I've set it to
ensure our jobs don't hang forever though.

Warts in this version:

- Rust thinks imports only used in the job body are unused.
  (sgrif/swirl#6)
  - Worked around for now by moving the imports into the job body
- `swirl::Job` has to be imported by calling code
- Codegen assumes that `serde::Deserialize` and `serde::Serialize` are
  available. (sgrif/swirl#9)

Issues with this version:

- The new impl of `run_all_pending_jobs` will return an error if the DB
  is in read only mode. If the DB is read only, we couldn't have
  enqueued jobs anyway, so the workaround is "fine", but we need some
  more robust APIs in swirl to fix this. This only affects us in tests.
  - sgrif/swirl#8
@sgrif
Copy link
Owner Author

sgrif commented Mar 18, 2019

Note: Fixing this is blocked on serde-rs/serde#1499 / serde-rs/serde#1487

bors added a commit to rust-lang/crates.io that referenced this issue Apr 1, 2019
Update swirl

New features in this version:

- Codegen for jobs which reduces boilerplate when defining them
- Jobs no longer need to be registered with the runner
- Rework of how `run_all_pending_jobs` works so errors loading the job
  actually bubble up
- Configurable timeout for how long to wait for a job to begin running

We aren't fully taking advantage of the timeout yet, I'd eventually like
to try rebuilding the runner in-process a few times when an error
happens, and then crash the process if it fails a few times in a row.
This needs a bit more refactoring though (I'm not sure if we want to
keep the `Repository` in memory like we are now, and whether we want to
assume a full re-clone is needed on error). For now I've set it to
ensure our jobs don't hang forever though.

Warts in this version:

- Rust thinks imports only used in the job body are unused.
  (sgrif/swirl#6)
  - Worked around for now by moving the imports into the job body
- `swirl::Job` has to be imported by calling code
- Codegen assumes that `serde::Deserialize` and `serde::Serialize` are
  available. (sgrif/swirl#9)

Issues with this version:

- The new impl of `run_all_pending_jobs` will return an error if the DB
  is in read only mode. If the DB is read only, we couldn't have
  enqueued jobs anyway, so the workaround is "fine", but we need some
  more robust APIs in swirl to fix this. This only affects us in tests.
  - sgrif/swirl#8
@sgrif sgrif closed this as completed in #12 May 16, 2019
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 a pull request may close this issue.

1 participant