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

implement batch task sending #52

Merged
merged 3 commits into from
Apr 15, 2017
Merged

implement batch task sending #52

merged 3 commits into from
Apr 15, 2017

Conversation

cenkalti
Copy link
Owner

No description provided.

@cenkalti cenkalti mentioned this pull request Apr 15, 2017
@coveralls
Copy link

coveralls commented Apr 15, 2017

Coverage Status

Coverage decreased (-0.2%) to 91.834% when pulling ced97d3 on batch into 7b4fbd2 on master.


def test_batch(self):
"""Batch tasks are run"""
li = [tasks.echo.subtask(args=("foo %i" % i, )) for i in range(2)]
Copy link
Contributor

Choose a reason for hiding this comment

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

While I see the point of having args and kwargs explicitly, it doesn't look as nice as just:

li = [tasks.echo.subtask("foo %i" % i) for i in range(2)]

Celery went with this approach, but I am not sure how to fit the host argument in this case.

Copy link
Owner Author

Choose a reason for hiding this comment

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

If there wasn't a host parameter I would do the same with couldn't come up with better solution :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I have looked into Celery sources and realized that they have implemented it the same way. In fact, they have two implementations:

  • .signature(args, kwargs) (.subtask is an alias to .signature)
  • .s(*args, **kwargs) (it, essentially, calls .signature(args=args, kwargs=kwargs)) + .si(*args, **kwrags) (they have a special case of "immutable" tasks)

kuyruk/task.py Outdated
@@ -194,6 +198,9 @@ def _module_name(self):
return name


SubTask = namedtuple("SubTask", "task, args, kwargs, host")
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 use a tuple instead of a string:

SubTask = namedtuple("SubTask", ("task", "args", "kwargs", "host"))

Is there any benefit of having them in a comma-separated string instead of a tuple or a list?

Copy link
Owner Author

Choose a reason for hiding this comment

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

No, they are the same. Just a different notation. Tuple looks better, I will update it.

tasks.py Outdated
kuyruk = Kuyruk()


@kuyruk.task(retry=5, fail_delay=10000, reject_delay=10000)
Copy link
Contributor

Choose a reason for hiding this comment

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

You have dropped fail_delay support recently.

@frol
Copy link
Contributor

frol commented Apr 15, 2017

Should the tasks.py and send.py be in the root of the project?

@cenkalti
Copy link
Owner Author

tasks.py and send.py are committed accidentally. I am removing those now.

@coveralls
Copy link

coveralls commented Apr 15, 2017

Coverage Status

Coverage decreased (-0.3%) to 91.691% when pulling 71306ea on batch into 7b4fbd2 on master.

@@ -53,6 +54,9 @@ def __call__(self, *args, **kwargs):
logger.debug("Task.__call__ args=%r, kwargs=%r", args, kwargs)
self.send_to_queue(args, kwargs)

def subtask(self, args=(), kwargs={}, host=None):
Copy link
Contributor

@frol frol Apr 15, 2017

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

I am aware of this issue but I am not mutating them inside the function.

@frol
Copy link
Contributor

frol commented Apr 15, 2017

I am really happy to see this new feature! @cenkalti Thank you!

@cenkalti cenkalti merged commit bf6ce6b into master Apr 15, 2017
@cenkalti cenkalti deleted the batch branch April 15, 2017 12:52
@cenkalti
Copy link
Owner Author

Thank you @frol. I have uploaded the new version: https://pypi.python.org/pypi/Kuyruk/8.3.0

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.

3 participants