framework icon indicating copy to clipboard operation
framework copied to clipboard

feat: refactor queue module

Open devhaozi opened this issue 2 years ago • 28 comments

Closes goravel/goravel#153 Closes goravel/goravel#278

📑 Description

This is a draft, need more discussion.

I tried to reconstruct the queue module with reference to Laravel's implementation. There are some issues that need to be discussed and determined.

✅ Checks

  • [ ] My pull request adheres to the code style of this project
  • [ ] My code requires changes to the documentation
  • [ ] I have updated the documentation as required
  • [ ] All the tests have passed

ℹ Additional Information

Summary by CodeRabbit

  • New Features

    • Introduced interface Driver for managing job queues with methods for pushing, popping, and scheduling jobs.
    • Added MigrateMakeCommand struct for creating migrations for failed queue jobs.
    • Implemented Shutdown method for the Worker to allow graceful shutdown.
    • Enhanced queue configuration with new methods like Via() and FailedJobsQuery().
  • Bug Fixes

    • Improved error handling in multiple methods across the application, including Register and Boot.
    • Standardized import statements and references for consistency.
  • Refactor

    • Updated structs and method signatures across various files to use more generic types and new field names.
    • Replaced machinery with driver in Worker and Task structs for better integration.
    • Modified SQL statements for database consistency.
  • Tests

    • Adjusted test cases to reflect new configurations and updated struct fields.
    • Simplified argument passing mechanisms within the test functions.
  • Documentation

    • Updated references in import statements for clarity and consistency.
    • Revised comments to reflect new method signatures and struct fields.

devhaozi avatar Oct 07 '23 18:10 devhaozi

cc @hwbrzzl

devhaozi avatar Oct 07 '23 18:10 devhaozi

@hwbrzzl Please help me check if there are any problems with the implementation idea. If there are no problems, I will continue to complete other driver development.

devhaozi avatar Oct 09 '23 08:10 devhaozi

Thanks for this, was trying to implement AWS SQS into a potential replacement product with goravel. And found that adding my own driver wasn't an option. Anyway keep it up! 💪

SwenVanZanten avatar Nov 17 '23 16:11 SwenVanZanten

Codecov Report

Attention: Patch coverage is 50.47022% with 158 lines in your changes missing coverage. Please review.

Project coverage is 70.26%. Comparing base (f2b9567) to head (b71cfeb). Report is 34 commits behind head on master.

Files with missing lines Patch % Lines
queue/console/migrate_stubs.go 25.00% 45 Missing :warning:
database/console/migrate_stubs.go 13.63% 19 Missing :warning:
queue/console/migrate_make_command.go 45.16% 15 Missing and 2 partials :warning:
queue/driver_sync.go 28.57% 13 Missing and 2 partials :warning:
queue/job.go 47.82% 11 Missing and 1 partial :warning:
queue/worker.go 66.66% 8 Missing and 3 partials :warning:
queue/driver.go 57.14% 9 Missing :warning:
queue/driver_async.go 79.54% 8 Missing and 1 partial :warning:
queue/service_provider.go 0.00% 6 Missing :warning:
queue/application.go 75.00% 4 Missing and 1 partial :warning:
... and 5 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #314      +/-   ##
==========================================
- Coverage   70.34%   70.26%   -0.09%     
==========================================
  Files         180      183       +3     
  Lines       11212    11202      -10     
==========================================
- Hits         7887     7871      -16     
- Misses       2751     2763      +12     
+ Partials      574      568       -6     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Dec 05 '23 17:12 codecov[bot]

This PR is basically completed, please let me know if there are any areas that need improvement.

Known issues:

  1. Database driver lacks test cases.
  2. Features related to task timeout have not been implemented yet.
  3. Features related to retry failed task have not been implemented yet.
  4. Database migration create command have not been implemented yet.

cc @hwbrzzl

devhaozi avatar Dec 05 '23 19:12 devhaozi

@devhaozi Great job man, do we cover all current features?

hwbrzzl avatar Dec 06 '23 07:12 hwbrzzl

@devhaozi Great job man, do we cover all current features?

Yes

devhaozi avatar Dec 06 '23 07:12 devhaozi

@devhaozi Great job man, do we cover all current features?

Yes

Laravel support this https://learnku.com/docs/laravel/10.x/queues/14873#8dd0bc and https://learnku.com/docs/laravel/10.x/queues/14873#6a8bef.

Currently, I have reserved relevant interfaces and methods, but they have not been fully implemented yet.

devhaozi avatar Dec 06 '23 07:12 devhaozi

Got it, will take a look.

hwbrzzl avatar Dec 06 '23 07:12 hwbrzzl

@hwbrzzl If redis BLPop timeout setting to 60 seconds, this test case will fail. image

devhaozi avatar Dec 17 '23 11:12 devhaozi

@hwbrzzl If redis BLPop timeout setting to 60 seconds, this test case will fail.

Why?

hwbrzzl avatar Dec 19 '23 08:12 hwbrzzl

@hwbrzzl If redis BLPop timeout setting to 60 seconds, this test case will fail.

Why?

image image

The logic here is similar to Laravel. First, need to merge the tasks in the delay queue into the main queue.

Next, BLPop will block until there is a task in the main queue or the timeout is reached.

Therefore, if it is set to 60s, it will always be blocked, causing the test to fail.

devhaozi avatar Dec 19 '23 11:12 devhaozi

The timeout parameter means an error will be thrown if something spends more time, right? Why will block?

hwbrzzl avatar Dec 19 '23 14:12 hwbrzzl

The timeout parameter means an error will be thrown if something spends more time, right? Why will block?

No, no, no, this is a blocking method, it will block until there is a task in the queue list or the timeout is reached. This is not specific to task running time, in fact, no control over task running time is currently implemented.

image

devhaozi avatar Dec 19 '23 14:12 devhaozi

@devhaozi I see, thanks!

hwbrzzl avatar Dec 20 '23 13:12 hwbrzzl

Hi @devhaozi Do you still work on this, please? The PR is a bit complex and spent a lot of time, I think we only need to implement the Redis driver for now, the DB driver is optional.

hwbrzzl avatar Mar 11 '24 03:03 hwbrzzl

Hi @devhaozi Do you still work on this, please? The PR is a bit complex and spent a lot of time, I think we only need to implement the Redis driver for now, the DB driver is optional.

I'm busy with other work, you can take over this PR if you have time.

devhaozi avatar Mar 11 '24 05:03 devhaozi

Hi @devhaozi Do you still work on this, please? The PR is a bit complex and spent a lot of time, I think we only need to implement the Redis driver for now, the DB driver is optional.

I'm busy with other work, you can take over this PR if you have time.

I think only need to implement sync and async here, and consider splitting database and redis into two packages.

devhaozi avatar Mar 11 '24 05:03 devhaozi

I will test this feature in my project.

devhaozi avatar May 29 '24 10:05 devhaozi

After remove machinery and sonic, the binary size has been reduced by about 50MB.

image

image

devhaozi avatar May 29 '24 10:05 devhaozi

After the discussion in the group, I think a goravel/machinery is necessary, we need to implement the driver interface in this PR through machinery, otherwise, users will hard to upgrade the framework in the future. Another way is createing a migrate command: machinery -> db or redis, but we cannot guarantee that it will be compatible with all situations.

hwbrzzl avatar Jun 02 '24 03:06 hwbrzzl

After the discussion in the group, I think a goravel/machinery is necessary, we need to implement the driver interface in this PR through machinery, otherwise, users will hard to upgrade the framework in the future. Another way is createing a migrate command: machinery -> db or redis, but we cannot guarantee that it will be compatible with all situations.

I prefer the second way.

devhaozi avatar Jun 02 '24 14:06 devhaozi

I prefer the second way.

If so, we need to check the machinery code for how to generate a structure from the queue, then transform the structure to ours.

hwbrzzl avatar Jun 03 '24 09:06 hwbrzzl

[!IMPORTANT]

Review skipped

Auto reviews are limited to specific labels.

Labels to auto review (1)
  • 🚀 Review Ready

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The changes focus on enhancing the queue system by introducing new methods and modifying existing ones, refining job handling, improving error handling, and expanding the trigger events in the GitHub workflow. Additionally, significant updates to dependencies and database migration stubs were made to ensure consistency and support for asynchronous job processing.

Changes

File(s) Change Summary
.github/workflows/pr-check-title.yml Switched trigger from pull_request to pull_request_target and refined event triggers.
contracts/queue/driver.go Introduced Driver interface with methods for job queue management.
contracts/queue/job.go Updated Jobs struct to include a Delay field.
contracts/queue/queue.go Modified Queue interface methods and added new ones, including GetJob and Shutdown.
contracts/queue/task.go Changed Delay method signature in Task interface from time.Time to uint.
database/console/migrate_stubs.go Modified SQL statements for consistency across database platforms.
event/application.go Added error handling in Register method of Application struct.
event/service_provider.go Renamed package eventConsole to eventconsole and updated references.
event/task.go Simplified conversion process from event.Arg to any type.
event/task_test.go Updated test function to use any type for job handling.
go.mod Updated dependencies, including removal of machinery and addition of do package.
mail/application.go Modified argument passing in Queue method to use any type.
mail/application_test.go Restructured test suite logic and configurations for mail and queue operations.
mail/service_provider.go Added error handling in Boot method of ServiceProvider struct.
queue/application.go Made changes to Application struct, including new methods and modifications to existing ones.
queue/config.go Updated methods in Config struct for queue configuration.
queue/config_test.go Modified configuration settings and test cases for queue connections.
queue/console/migrate_make_command.go Added MigrateMakeCommand struct for creating migration for failed queue jobs database table.
queue/driver_async.go Introduced functionality for asynchronous job processing.
queue/task.go Major changes to Task struct, including field types and method signatures.
queue/task_test.go Updated TestDispatchSync function to use JobImpl for job registration.
queue/utils_test.go Renamed import queuecontract to contractsqueue and updated references.
queue/worker.go Updated Worker struct fields and methods, including new Shutdown method and asynchronous job processing.

Assessment against Linked Issues

Objective Addressed Explanation
Refactor the queue facades (#153)
Implement async and sync job dispatch, Redis/Database/memory driver (#153)
Provide interfaces to get queue status (#153)
Fix queue always printing DEBUG messages with Redis driver (#278) Needs review to confirm if debug messages issue has been resolved.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

coderabbitai[bot] avatar Jul 10 '24 15:07 coderabbitai[bot]

Currently, machinery has resumed maintenance, so this PR may no longer be needed.

devhaozi avatar Aug 21 '24 16:08 devhaozi

machinery still doesn't support the DB driver, right?

hwbrzzl avatar Aug 24 '24 02:08 hwbrzzl

machinery still doesn't support the DB driver, right?

Yes

devhaozi avatar Aug 24 '24 08:08 devhaozi

If so, I think it's better to implement the DB driver ourselves.

hwbrzzl avatar Aug 25 '24 14:08 hwbrzzl