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

Add least processing time distribution algorithm #1

Open
wants to merge 4 commits into
base: class_distribution
Choose a base branch
from

Conversation

plboerboom
Copy link

No description provided.

Copy link

@simon-weber simon-weber left a comment

Choose a reason for hiding this comment

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

looking good! left a minor comment about the error handling

"'--lpt-data <lpt-data-filepath>' to be specified as "
"well. Falling back to hash-ring algorithm."
))
self.algorithm = self.ALGORITHM_HASH_RING

Choose a reason for hiding this comment

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

thoughts on just crashing here+below instead of falling back? my guess is it's intended to defend against things higher up in the stack (eg failing to download a data file), which does seem like what we want but might not be the best place to handle it

Copy link
Author

Choose a reason for hiding this comment

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

right, good point. lemme give that some thought.

Copy link
Author

Choose a reason for hiding this comment

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

actually, yeah, falling back is definitely bad in the case of the failed download since that might have happened at only one node and then the different nodes might successfully execute different algorithms.

good call. i'll revise this to just crash and fail for these cases.

Choose a reason for hiding this comment

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

ah, yeah, good point. fwiw, so long as nose doesn't write out a nosetests.xml our builds will consider it unstable, so we could hook up retries at that level if needed.

@plboerboom plboerboom force-pushed the lpt_distribution branch 3 times, most recently from 25dad61 to afbd403 Compare November 29, 2017 18:25
@plboerboom
Copy link
Author

hey @simon-weber -- made some error handling updates based on your comments. can you take another look?

Copy link

@simon-weber simon-weber left a comment

Choose a reason for hiding this comment

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

looks good!

@plboerboom plboerboom force-pushed the lpt_distribution branch 2 times, most recently from cc8617a to 8ee3ccc Compare December 14, 2017 16:27
@simon-weber
Copy link

lgtm 👍

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.

2 participants