Skip to content
This repository has been archived by the owner on Aug 13, 2024. It is now read-only.

Release/0.5.0 #102

Closed
wants to merge 4 commits into from
Closed

Release/0.5.0 #102

wants to merge 4 commits into from

Conversation

ungn
Copy link

@ungn ungn commented Feb 20, 2017

Factotum "server mode":

  • http server
  • json
    • parse
    • construct
    • validate
  • state
    • log file/database
    • consul
  • dispatcher
    • one-to-many queue
    • add to queue
    • get queue status
  • worker process job
  • cli
  • merge into main factotum Cargo.toml move to separate repo
  • ci/cd
  • docs

@ungn ungn added this to the Version 0.5.0 milestone Feb 20, 2017
@ungn ungn self-assigned this Feb 20, 2017
@alexanderdean
Copy link
Contributor

It's coming along @ungn!

Copy link
Contributor

@ninjabear ninjabear left a comment

Choose a reason for hiding this comment

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

Hey @ungn this isn't really a review just the notes from our chat - looking good!

persistent = "0.3"
logger = "0.2"
env_logger = "0.3"
rustc-serialize = "0.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

let mut writer = rwlock.write().unwrap();
if writer.is_running() {
let mut body = String::new();
match request.body.read_to_string(&mut body) {
Copy link
Contributor

Choose a reason for hiding this comment

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

impl Dispatcher {
pub fn new() -> Dispatcher {
Dispatcher {
job_queue: Vec::new(),
Copy link
Contributor

Choose a reason for hiding this comment

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

vec![]

@ninjabear
Copy link
Contributor

Hey @ungn, just to recap my thoughts on what the "thread pool/channels" part of this will look like.

At the beginning of the process we create a new thread that will be our "worker manager". We can communicate with this thread using a channel. Each request that comes in should be transformed into a strongly typed request for the "worker manager" by the iron handlers (this means our "worker manager" can be unit tested without being coupled to the actual http requests).

This "worker manager" is responsible for:

  1. ensuring no more than N jobs are running at once (others are queued)
  2. providing information on how many jobs are queued and so on.

Jobs themselves can be dispatched on futures provided by this crate.

We may not need a separate thread to process a channel (or even need a channel at all). My only reason for including it is so we can dequeue jobs as running jobs are completed (separate from http calls).

So, a bit more on "seams" and why I always ramble about them, the Iron request handlers are responsible for converting HTTP requests into some kind of request understandable by our business logic. This means that we can test the business logic without iron. We should be able to test the request handlers convert the right kind of data into business logic requests, also without iron.

What we don't need to do however is spin up iron and see that our requests "really" turn into jobs and so on. It'd be nice to add this level of integration testing - entirely optional. I'm on the "tests should run faster than facebook.com loads" kind of camp. Apologies if I'm teaching you to suck eggs here!

It's cool if through looking at this in more detail you come up with something better. I just wanted to explain one way that could work - the only bad plan is one that cannot be changed imo.

@ungn ungn force-pushed the release/0.5.0 branch 2 times, most recently from 373d453 to 83340cf Compare March 16, 2017 11:20
@ungn ungn force-pushed the release/0.5.0 branch 2 times, most recently from 0bfcea2 to e4829a6 Compare March 16, 2017 14:48
@ungn ungn force-pushed the release/0.5.0 branch 2 times, most recently from 7c8a49f to 1efe674 Compare March 22, 2017 17:09
@ungn
Copy link
Author

ungn commented Mar 23, 2017

TODO:

  • add default values for cmd line args
  • update app logging:
    • change from file to stdout/err
    • add cmd line arg for absolute log file path (if unable to change)
    • add cmd line arg for log level
  • ignore jobId field in job request when parsing json (should not be required as it is generated)
  • remap rust snake_case fields to json camelCase using serde
  • remove system stats from status message
  • read in request url query string (pretty=1, id params, etc)
  • return better error messages (possible iron framework limitation that needs workaround)
  • add start time and uptime stats

@ungn
Copy link
Author

ungn commented Jun 5, 2017

Closing as changes for #98 are covered in separate factotum-server project and #94 is parked until a better solution for underlying problem can be found (see comments for further info).

@ungn ungn closed this Jun 5, 2017
@ungn ungn deleted the release/0.5.0 branch June 5, 2017 15:10
@ungn ungn removed this from the Version 0.5.0 milestone Jun 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants