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

feat(worker): added worker state. #396

Merged
merged 1 commit into from
Mar 13, 2018
Merged

Conversation

VoyTechnology
Copy link
Member

This currently contains one method which manages updating the task and
removing the pending tasks.

Related to #393

@VoyTechnology
Copy link
Member Author

Related to #375 (Rewrite worker as broker consumer)


use super::*;

use cerberus_proto::datatypes::{Task, TaskKind, TaskStatus};
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this should be:

use cerberus_proto [...]
use super::*;

pub fn new(path: &PathBuf) -> Result<Self, StateError> {
Ok(FileStore {
path: path.clone(),
jobs_path: [&path, &PathBuf::from(JOBS_DIR)].iter().collect(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this use Path::join?

pub enum StateErrorKind {
#[fail(display = "Failed to connect to state store server.")]
ConnectionFailed,
#[fail(display = "Failed precondition")]
Copy link
Contributor

Choose a reason for hiding this comment

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

This error message is too vague.

TaskSerialisationFailed,
#[fail(display = "Failed to deserialise the task proto.")]
TaskDeserialisationFailed,
#[fail(display = "Failed operation.")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

@@ -23,53 +22,50 @@ impl FileStore {
pub fn new(path: &PathBuf) -> Result<Self, StateError> {
Ok(FileStore {
path: path.clone(),
jobs_path: [&path, &PathBuf::from(JOBS_DIR)].iter().collect(),
jobs_path: path.clone().join(JOBS_DIR),
Copy link
Contributor

Choose a reason for hiding this comment

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

There shouldn't be a need for clone before join as join just takes a reference to self and returns a new object.

Copy link
Contributor

@GoldenBadger GoldenBadger left a comment

Choose a reason for hiding this comment

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

Please don't rebase during code review.

@VoyTechnology
Copy link
Member Author

Sorry, I thought it was only squash that was bad.

@VoyTechnology VoyTechnology force-pushed the feature/worker-state branch 2 times, most recently from 641ef84 to a4cb87a Compare March 13, 2018 18:13
This currently contains one method which manages updating the task and
removing the pending tasks.
@VoyTechnology VoyTechnology merged commit f8f83a8 into develop Mar 13, 2018
@VoyTechnology VoyTechnology deleted the feature/worker-state branch March 13, 2018 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants