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

Extended djhuey to support multiple huey instances #230

Merged
merged 4 commits into from Feb 28, 2018

Conversation

SeverinAlexB
Copy link

Heey guys

We use huey with django in our production system and we need independent huey instances to process different tasks. Unfortunately the huey django plugin only supported one huey instance so I reworked the django plugin (djhuey).

What changed

  • HUEY configuration supports multiple huey instances.
  • 'python manage.py run_huey' has a additional parameter to indicate on which huey instance it will consume.
  • This change is backward compatible. Old configurations can still be used. No code has to be changed to use these new features.
  • Updated the docs.

Configuration example

HUEY = {
    'my-app': {
        'default': True,  # Indicates the default huey instance
        'backend': 'huey.backends.redis_backend',
        'connection': {'host': 'localhost', 'port': 6379},
            'consumer': {
                'workers': 4,
                'worker_type': 'process',
        }
    },
    'my-app2': {
        'backend': 'huey.backends.sqlite_backend',
        'connection': {'location': 'sqlite filename'},
            'consumer': {
                'workers': 4,
                'worker_type': 'process',
        }
    },
}

The old configuration style is still valid. It only doesn't support multiple huey instances.

run_huey example

python manage.py run_huey --queue my-app2 
python manage.py run_huey // Runs a consumer on the default huey instance

Define task example

from huey.contrib.djhuey import task

@task('my-app2')
def example_task(param):
        print(param)

@task()  # Uses the default huey instance.
def example_task2(param):
        print(param)

We currently use my huey branch in our production system and it is thus already quiet mature.
Don't hesitate to contact me for small changes.

@coleifer
Copy link
Owner

Wow, this is quite clean. Thanks. I will review this as soon as I can with an eye towards getting it merged into master.

@winkidney
Copy link
Contributor

Great work!
Since it is a breaking change, would you like to provide a method to keep compatible of the old version?

Best Wishes

@SeverinAlexB
Copy link
Author

@winkidney All the old configurations are still working with this pull request. It only adds a new way to use several huey instances. Thus it is not a breaking change.

@winkidney
Copy link
Contributor

@Sebubu Sorry I missed it. It helps a lot : )

@SeverinAlexB
Copy link
Author

SeverinAlexB commented Jul 12, 2017

This commit above fixes a bug in djhuey which always started the consumer with one thread.
I really don't know why this bug fix lets one test fail. The changes have nothing to do with the main huey code.

@camilonova
Copy link
Contributor

Looks awesome @Sebubu 🍺

@coleifer
Copy link
Owner

Yes! It does! I'm sorry I have been so flakey about going over this. Thanks for your patience.

@Gasoid
Copy link

Gasoid commented Jan 31, 2018

@Sebubu Is it stable? We are looking forward this PR))

@ilyinon
Copy link

ilyinon commented Jan 31, 2018

It looks amazing!

@coleifer Could you please answer, are you planning to merge it into master?

@coleifer
Copy link
Owner

Apologies for the slowness... Could you rebase into a single commit, remove all "stylistic" (i.e. newlines/whitespace) changes, and resolve conflicts? When that's done I'm happy to review.

@SeverinAlexB
Copy link
Author

I can make that. I'm currently busy in another project but I try to do that till the 24. february.

Maybe I need your help briefly @coleifer in order to fix one test which didn't run the last time and I had no clue why.

@coleifer
Copy link
Owner

coleifer commented Feb 1, 2018

Thanks @Sebubu - I'm sorry for letting this sit idle and accumulate conflicts. I'm happy to help with failing tests once you submit a clean/rebased patch.

Added testing env for djhuey

added ConfigurationReaders

added default configuration

added DjangoHuey

refactored readers

further refactorings

added argument to run_huey

added queue parameter for run_huey

updated docs

added title to docs

removed print

Update django.rst

fixed run_huey bug

added check if huey is in settings

some adjustments

install test_requirements.txt in travis ci

added python2.7 compatibality

fixed threading bug
# Conflicts:
#	docs/django.rst
#	huey/contrib/djhuey/__init__.py
#	huey/contrib/djhuey/management/commands/run_huey.py
#	runtests.py
@MarcoGlauser
Copy link
Contributor

As @Sebubu is on holiday, I took over this PR.
@coleifer I did a rebase patch and merged your master.
But the tests are failing and i cannot figure out why because the PR only changes files in djhuey. can you point me into the right direction?

@MarcoGlauser
Copy link
Contributor

after some more digging I found the culprit. The problem was that the django test runner also tried running all non django tests and failed. now that i configured the django runner only to discover djhuey test it works.

@coleifer
Copy link
Owner

Very cool, will give it a review so we can get this merged.

@coleifer
Copy link
Owner

Looks good. A few small things here and there, but I can clean them up after merging. Thanks!

@coleifer coleifer merged commit a5db951 into coleifer:master Feb 28, 2018
@coleifer
Copy link
Owner

coleifer commented Feb 28, 2018

Clean-ups: cfdc87c

@coleifer
Copy link
Owner

@MarcoGlauser / @Sebubu -- are you all running this in production with the new queue option? There have been a couple brain-dead bugs that I think would've cropped up if you all were using this:

I don't want to revert this, but am concerned about the stability of the changes that were introduced...especially given the types of issues people are encountering.

coleifer added a commit that referenced this pull request Mar 12, 2018
@coleifer
Copy link
Owner

I've decided to roll this patch back and have pushed a new release, 1.9.0.

I apologize for the inconvenience this may cause you all, but I can't maintain a patch that has bugs like the two that were reported. If you want to re-submit a clean patch that addresses the reported issues and includes a more robust test-suite, I'll be happy to bring this functionality back.

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

7 participants