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

[17.0][FIX] Refactor stop signal handling in QueueJobRunner #647

Closed
wants to merge 25 commits into from

Conversation

Maralai
Copy link

@Maralai Maralai commented May 7, 2024

This commit addresses the issue: #564.

Refactor QueueJobRunner to Enhance Performance, Signal Handling, and Stability

This pull request brings substantial improvements and new functionalities to the QueueJobRunner, which is focused on optimizing performance, signal handling, and stability. The significant changes include the transition to asynchronous HTTP requests, dynamic detection of new databases, and enhancements for robust signal handling.

Key Changes:

  1. Async HTTP Requests:

    • Transitioned from threading-based requests to asyncio and aiohttp for handling HTTP job runs asynchronously. This change enhances concurrency and network-bound operation efficiency.
  2. Socket Pair for Signal Handling:

    • Implemented socket pairs to effectively manage stop signals, ensuring compatibility across UNIX and Windows environments.
  3. Selectors for Notification Handling:

    • Adopted selectors.DefaultSelector() for handling database and stop signal notifications, improving efficiency and scalability of notification handling.
  4. Auto-Detection of New Databases:

    • Added functionality for periodically checking and initializing newly created databases with the queue_job module installed, allowing dynamic adaptation without server restart.
  5. Configurable Check Interval:

    • Introduced a parameter for setting the check interval for new databases, enabling a balance between responsiveness and system overhead.
  6. Database Keep-Alive Mechanism:

    • Added periodic SELECT 1 queries to keep database connections alive and prevent intermediaries from closing idle connections.

Please review the changes and provide feedback or approval for merging this enhanced version of the QueueJobRunner. These improvements address the aforementioned issue and the todo's included within the original file, increasing robustness, scalability, and overall performance. I have included test cases and attempted to comply with pre-commit tests, with the aim of achieving a green checkmark.

Thank you for your consideration.

This commit refactors the stop signal handling in the `QueueJobRunner` class. Instead of using a pipe, it now uses a socket pair to initialize the stop signals. This change improves the code readability and ensures compatibility with Windows by manually creating a socket pair when the `socketpair` function is not available.
@OCA-git-bot
Copy link
Contributor

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

@Maralai Maralai changed the title 17.0 Refactor stop signal handling in QueueJobRunner [17.0][FIX] Refactor stop signal handling in QueueJobRunner May 26, 2024
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Sep 29, 2024
@github-actions github-actions bot closed this Nov 3, 2024
@Maralai
Copy link
Author

Maralai commented Nov 3, 2024

This is too bad. @guewen is there a reason that this improvement wouldn't be considered?

@sbidoul
Copy link
Member

sbidoul commented Nov 3, 2024

Hi @Maralai. Thanks for the contrib. This looks interesting but maybe too many changes at the same time. I'd recommend submitting as small incremental PR's that would be easier to review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants