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

[IMP] queue_job_cron_jobrunner: channel #750

Draft
wants to merge 3 commits into
base: 14.0
Choose a base branch
from

Conversation

Koudja
Copy link

@Koudja Koudja commented Mar 5, 2025

Implement channel on queue_job_cron_jobrunner

@OCA-git-bot
Copy link
Contributor

Hi @ivantodorovich,
some modules you are maintaining are being modified, check this out!

@Koudja Koudja force-pushed the 14.0-foodles-queue_job_cron_jobrunner-channel branch 3 times, most recently from 656b232 to 70b10c4 Compare March 11, 2025 15:23
Copy link

@alexandregaldeano alexandregaldeano left a comment

Choose a reason for hiding this comment

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

Once the tests are fixed, LGTM! 🚀

Comment on lines +49 to +55
channels = {}
for queue_job in self.search([("state", "=", "started")]):
if not queue_job.channel:
continue
channels[queue_job.channel] = channels.get(queue_job.channel, 0) + 1

Choose a reason for hiding this comment

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

suggestion: what do you think?

Suggested change
channels = {}
for queue_job in self.search([("state", "=", "started")]):
if not queue_job.channel:
continue
channels[queue_job.channel] = channels.get(queue_job.channel, 0) + 1
channels = defaultdict(int)
for queue_job in self.search([("state", "=", "started")]):
if not queue_job.channel:
continue
channels[queue_job.channel] += 1

result = queue_job
break
self.flush()
self.env.cr.commit() # pylint: disable=E8102

Choose a reason for hiding this comment

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

suggestion: Maybe add a comment to explain why you can't use a different cursor (lock never released by Odoo)?

@Koudja Koudja force-pushed the 14.0-foodles-queue_job_cron_jobrunner-channel branch 3 times, most recently from 53454b8 to d72fbca Compare March 12, 2025 17:46
petrus-v added a commit to petrus-v/queue that referenced this pull request Mar 13, 2025
this make a lot of open file descriptors that we got the limit while instentiate
jobrunner from queue_job_cron_jobrunner in the current channel implementation
OCA#750
petrus-v added a commit to petrus-v/queue that referenced this pull request Mar 14, 2025
this make a lot of open file descriptors that we got the limit while instentiate
jobrunner from queue_job_cron_jobrunner in the current channel implementation
OCA#750
Copy link
Contributor

@petrus-v petrus-v left a comment

Choose a reason for hiding this comment

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

Nice one, thanks !

@petrus-v
Copy link
Contributor

This PR require #754 to avoid opened file descriptor leak !

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

petrus-v added a commit to petrus-v/queue that referenced this pull request Mar 14, 2025
this make a lot of open file descriptors that we got the limit while instentiate
jobrunner from queue_job_cron_jobrunner in the current channel implementation
OCA#750
@petrus-v petrus-v force-pushed the 14.0-foodles-queue_job_cron_jobrunner-channel branch from c6ff271 to 918702c Compare March 20, 2025 21:25
@hbrunn
Copy link
Member

hbrunn commented Mar 21, 2025

@Koudja this is still in draft state, is this intentional?

@petrus-v
Copy link
Contributor

@Koudja this is still in draft state, is this intentional?

In fact we are still supervising the system and keep improving it as long we met difficulties ! yesterday we've implement the stop_processing feature to avoid to be kill by the cpu time limit in case there is a lot of job in the queue, this is not yet deployed to our production (and I'm going to somme logging).

So I suppose keeping it in draft state as long we have not get some stabilities is probably better ! But review and feedback are welcome otherwise so not sure what suits most OCA conventions in such case ?

@petrus-v petrus-v force-pushed the 14.0-foodles-queue_job_cron_jobrunner-channel branch from 918702c to 1c94494 Compare March 21, 2025 09:18
@hbrunn
Copy link
Member

hbrunn commented Mar 21, 2025

draft is good I think, because of the label I nearly merged it

"queue_job_cron_jobrunner.stop_processing_threshold_seconds", "5"
)
)
if next_cron_job_runner_trigger.nextcall <= (
Copy link
Contributor

Choose a reason for hiding this comment

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

after few investigation, I've the feeling the nextcall is not yet uppdated so we are probably dealing with the current started date so we would have to compute it...

Copy link
Contributor

Choose a reason for hiding this comment

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

because odoo changed it on a different cursor !...

@sbidoul
Copy link
Member

sbidoul commented Mar 21, 2025

I'm wondering... now that the job scheduler (misnamed jobrunner) can run on odoo.sh (#668), queue_job_cron_jobrunner could maybe be simplified to only run jobs that are in state enqueued, and keep all the scheduling logic (channels, priorities, eta, etc) in the regular jobrunner ?

…job list

wait the next cron trigger instead to avoid to reach the
limit_time_real_cron limit.
@petrus-v petrus-v force-pushed the 14.0-foodles-queue_job_cron_jobrunner-channel branch from 1c94494 to 867608d Compare March 21, 2025 15:04
@petrus-v
Copy link
Contributor

petrus-v commented Mar 21, 2025

I'm wondering... now that the job scheduler (misnamed jobrunner) can run on odoo.sh (#668), queue_job_cron_jobrunner could maybe be simplified to only run jobs that are in state enqueued, and keep all the scheduling logic (channels, priorities, eta, etc) in the regular jobrunner ?

As of today, we are not using the mentioned patch and have configured our setup to avoid spawning the job scheduler/runner, as suggested in the module’s documentation.

To be honest, we haven’t spent much time evaluating this patch, so we might have missed some key information. However, we do have some concerns:

  • A preference for using a cron worker to process queue jobs.
  • Uncertainty about how Odoo.sh manages process lifecycles, especially regarding jobrunner. If it gets killed (due to inactivity, lack of HTTP requests, or other obscure platform limits), how does the system handle it? Would it be properly restarted by Odoo.sh?

If I understand correctly, your idea is to have the queue jobrunner focus on managing job states—ensuring that tasks are enqueued while respecting channel capacities—while letting the ir.cron execute them based on priority. This approach would remove the need for HTTP workers to process jobs.

That sounds like an interesting direction and avoid to duplicate the channel implementation logic. Would you be open to further designing this together or discussing the long-term vision for the module?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants