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

Change priority to use zpriority and change default priority to +inf #5

Merged
merged 2 commits into from Feb 14, 2017

Conversation

AJRenold
Copy link
Contributor

For @sbarman

cc - @yorinasub17

Changes:

  • Change priority to zpriority to avoid confusion with other celery priority implementations
  • Change default priority to +inf

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 44.007% when pulling 9abe187 on aj-update-priority into dce2b36 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 44.007% when pulling b51dfbf on aj-update-priority into dce2b36 on master.

@@ -3,7 +3,7 @@
setup(
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to update the download_url too

README.md Outdated

## Using with celery tasks - zpriority

task.apply_async(zpriority=10)
Copy link
Contributor

Choose a reason for hiding this comment

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

That's pretty crazy that you can straight pass through celery a new option like this.

Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

Aside from one comment about version, this looks good. A bit uncomfortable about relying on a hidden feature in celery where message properties can be set.

@sbarman
Copy link
Contributor

sbarman commented Feb 13, 2017

@yorinasub17 I can see that. But I think using "priority" is worse since the ranges of priority differ and the meaning of priorities differ (in normal priority, larger means high priority). In this case, even if we switch backends, then the worst thing that happens is that we switch back to FIFO.

@yorinasub17
Copy link
Contributor

@sbarman agreed that this is the better of the two evils.

@yorinasub17 yorinasub17 mentioned this pull request Feb 13, 2017
@coveralls
Copy link

coveralls commented Feb 14, 2017

Coverage Status

Coverage increased (+0.1%) to 44.103% when pulling d73e601 on aj-update-priority into 903e3e5 on master.

@coveralls
Copy link

coveralls commented Feb 14, 2017

Coverage Status

Coverage increased (+0.1%) to 44.103% when pulling 5960e73 on aj-update-priority into 903e3e5 on master.

@AJRenold AJRenold merged commit 4f055d0 into master Feb 14, 2017
Copy link
Contributor

@sbarman sbarman 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 overall. Did you tag this commit with the version? We will also need to push out a release to PyPi

README.md Outdated

zpriority is used to avoid confusion with other celery priority implementations

Note - tasks created using this backend will have the lowest priority, +inf
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add "tasks created without a zpriority" just to clarify its the default

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.

None yet

4 participants