-
Notifications
You must be signed in to change notification settings - Fork 96
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
Separate Task/Queue for SearchIndex #59
Comments
@jobelenus I don't have any experience using celery-haystack, but I have plenty using celery. I handle different priority queues in some of my own applications, so I can't imagine it being a huge problem here - I just need to know where to start looking. Can you point me to where the current bottleneck is and where you'd expect the change to take place? |
Sure. Starting in the signal processor: https://github.com/django-haystack/celery-haystack/blob/develop/celery_haystack/signals.py#L44 And this is the Where Let me know if thats confusing |
Since celery doesn't support message priority I have to separate out the messages into different queues and then allocate the amount of resources I want to each queue. Correct? |
Yes. You'll basically want to send your special indexes to a different queue(s) than the rest so they end up getting processed faster. I've briefly looked at the code and I'm not sure the best approach. What were you thinking your approach would be? |
That is precisely what I was thinking. More than happy to do the work. Just On Wed, Jul 27, 2016 at 6:08 PM, Steve Byerly [email protected]
|
I think it's a reasonable addition. Go for it. |
Hrmm, this is odd... while testing I'm getting this trace. I've forked off master and made my changes, but something seems to be not quite write with the app config? The
Edit: I thought I figured out what was going on and imported the celery_haystack conf settings in my own application task, but it didn't quite do the trick:
I can see that Edit2: Actually even without that settings import my own task I happen to be in Django 1.9.6 if that makes a difference, I see the travis config has 1.9 |
It looks like an issue with app loading and your new tasks. When i run celery_haystack locally, I can import the settings fine. If you add |
I can do When I had |
I believe it gets loaded when django does its magic app loading. I believe Celery also does some loading of tasks too. What happens when you manually add the fully qualified setting ( |
Well of course that will work, if i add the setting -- but that isn't really a solution to the problem, and there are at least 3 settings that the utils.py file needs. What is really weird is that the indexes.py file seems to work fine with the app loading, but not utils. Thats what confuses me the most about this |
I thought perhaps this would fix it, but it doesn't jobelenus@2479424 (e.g. that the relative import was using my apps settings file instead of the celery_haystack) |
Can you post the new code you added to your own project? Either here or a gist? |
Sure, its really not much. I'm not even using the new feature yet.
When I create a new User is when I see the error. I didn't even put the |
Why are you using a custom signal handler if the goal is to define the task queues on the searchindex? Then instead of defining the routes, you can define the queues, though i don't think you have to - they'll get added by celery auto-magically:
|
So I can do this: But even if I did't make that class (I'm not even using it yet) this has a setup problem: The weird part is that |
Based on our initial comments, and this change: jobelenus@425f5be You would just add Then, the current signal handler will use the |
No. So my index change would be:
|
ah, right. i was thinking it was much simpler. Either way, I still think it's an issue with app loading between Django, Celery, and AppConf. Celery wants to create the celery app/tasks, Django wants to import all the install apps, then AppConf needs to be loaded at some point. I'm assuming there wouldn't be a problem if this project was not depending on AppConf. My earlier point about adding the setting that's throwing the AttributeError directly to the file was to show that everything runs w/o AppConf. |
If I do not import Why would importing in my app cause a problem within the |
OK -- small update: I did not realize that the conf.py file was using an outside appconf (i read it quickly and thought it was the Django AppConfig), egg on me. The issue is the application loading lifecycle. So I created an I also needed to change my own tasks file for application loading: I was loading my tasks in my |
Don't forget to add the following (specific per-app) that you're adding an apps.py file to.
https://docs.djangoproject.com/en/1.9/ref/applications/#configuring-applications |
Right, I went with the (now Django preferred) of |
@jobelenus so you got everything up and running? Are there any other changes needed for the PR? |
OK -- I finally figured out my other problem. I will polish this up and submit a PR |
Sorry its been a while here, was dealing with other fallout. I noticed that I can't squash the commits for the PR I want to submit (its much cleaner that way). Can you guys allow that options? If not thats OK. But I want to send you the PR. |
I have a lot of real time indexing going on in my application. There is one model that actually has a higher priority of being indexed. But these are a minority of indexing jobs going on in the background. Other jobs are ahead of it in the queue so it sits there waiting.
I can solve this problem by creating a separate route for a specific task of my own creation that will extend your task and do nothing different. The problem is how to get the signal processor to launch that task instead of your default task. I see the optional task path arg and the setting for the task. But the problem is that in enqueue doesn't pass an argument.
I was hoping to avoid copying that method in my own signal processor just to change one line for passing an argument. Could there be a class attribute on the signal processor that represents the task path?
Or do you have a better proposal for solving this specific problem. I can of course actually do the work and submit a PR - just hoping to get some communication here that this is the desired approach and that the PR would be accepted and get released fairly quickly?
The text was updated successfully, but these errors were encountered: