Page MenuHomePhabricator

[blob] Add CLI option to run cleanup task
ClosedPublic

Authored by bartek on Oct 9 2023, 5:38 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jun 15, 8:43 AM
Unknown Object (File)
Fri, Jun 14, 4:29 PM
Unknown Object (File)
Tue, Jun 11, 8:08 PM
Unknown Object (File)
Fri, Jun 7, 5:24 AM
Unknown Object (File)
Fri, Jun 7, 12:16 AM
Unknown Object (File)
Thu, Jun 6, 10:52 PM
Unknown Object (File)
Sat, May 25, 7:20 PM
Unknown Object (File)
May 4 2024, 6:24 PM
Subscribers

Details

Summary

Resolves ENG-4759.
Added CLI subcommand to choose if we want to start blob service server or run the cleanup task.

Depends on D9355

Test Plan
  • Existing CLI options for running server work the same - not providing any option starts the server
  • cargo run -- server works the same vay as above
  • cargo run -- cleanup starts the cleanup task

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Oct 9 2023, 6:03 AM
bartek added inline comments.
services/blob/src/main.rs
39–42 ↗(On Diff #31790)

With cleanup task, we no longer need to override this option.
This change means that blobs will no longer be deleted right after last holder was removed. Instead, they'll be marked as "unchecked" and deleted during cleanup job.

  1. Don't we need to update the command in the docker file?
  2. This way cargo run -- server --http-port 123 doesn't work, only cargo run -- --http-port 123 server works, which feels weird. We could potentially add global = true to all args but that also has it's problems
  1. Don't we need to update the command in the docker file?

No, Running server is the default. For cleanup, the command is overridden in terraform ecs task definition. However I can add the ["blob", "server"] to dockerfile for clarity

  1. This way cargo run -- server --http-port 123 doesn't work, only cargo run -- --http-port 123 server works, which feels weird. We could potentially add global = true to all args but that also has it's problems

Yeah, I'm aware of that. Also passing --http-port is allowed when running a cleanup task too (it's a no-op then).
A more idiomatic way is to make --http-port a subcommand property: enum Command { Cleanup, Server { #[arg(...)] pub http_port: u16 } }. But Idk if it allows to omit passing the server subcommand (breaking change).
Anyway, I'm leaning towards modifying it this way and accepting a breaking change (we're bumping major anyway 😛).

This revision is now accepted and ready to land.Oct 10 2023, 8:22 AM

Address feedback: Set --http-port as global, change dockerfile command

This revision was automatically updated to reflect the committed changes.