feat: refactor queue module #314
Conversation
queue/migrations/00000000000001_create_failed_jobs_table.up.sql
Outdated
Show resolved
Hide resolved
|
cc @hwbrzzl |
|
@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. |
# Conflicts: # go.mod # go.sum
…queue # Conflicts: # go.mod # go.sum
|
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe 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
Assessment against Linked Issues
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Outside diff range, codebase verification and nitpick comments (3)
event/task.go (1)
Line range hint
19-62:
Consider adding error handling for event handling and listener dispatching.The
Dispatchmethod should handle potential errors from event handling and listener dispatching.handledArgs, err := receiver.event.Handle(receiver.args) if err != nil { return err } var mapArgs []any for _, arg := range handledArgs { mapArgs = append(mapArgs, arg.Value) } for _, listener := range receiver.listeners { var err error task := receiver.queue.Job(listener, eventArgsToQueueArgs(handledArgs)) queue := listener.Queue(mapArgs...) if queue.Connection != "" { task.OnConnection(queue.Connection) } if queue.Queue != "" { task.OnQueue(queue.Queue) } if queue.Enable { err = task.Dispatch() } else { err = task.DispatchSync() } if err != nil { return err } } return nilqueue/worker.go (2)
12-18: Add documentation comments.Consider adding documentation comments for each attribute to improve readability and maintainability.
type Worker struct { // concurrent is the number of concurrent workers. concurrent int // driver is the driver implementation for the worker. driver *DriverImpl // job is the job implementation for the worker. job *JobImpl // failedJobs is the query for failed jobs. failedJobs orm.Query // queue is the name of the queue. queue string // failedJobChan is the channel for failed jobs. failedJobChan chan FailedJob // isShutdown indicates if the worker is shutting down. isShutdown bool }
57-63: Add documentation comments.Consider adding documentation comments for each attribute to improve readability and maintainability.
type FailedJob struct { // Queue is the name of the queue. Queue string // Signature is the job signature. Signature string // Payloads are the job arguments. Payloads []any // Exception is the error message. Exception string // FailedAt is the timestamp of the failure. FailedAt carbon.DateTime }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (5)
go.sumis excluded by!**/*.summocks/queue/Driver.gois excluded by!mocks/**mocks/queue/Queue.gois excluded by!mocks/**mocks/queue/Task.gois excluded by!mocks/**mocks/queue/Worker.gois excluded by!mocks/**
Files selected for processing (31)
- .github/workflows/pr-check-title.yml (1 hunks)
- contracts/queue/driver.go (1 hunks)
- contracts/queue/job.go (1 hunks)
- contracts/queue/queue.go (2 hunks)
- contracts/queue/task.go (1 hunks)
- database/console/migrate_stubs.go (8 hunks)
- event/application.go (1 hunks)
- event/service_provider.go (2 hunks)
- event/task.go (1 hunks)
- event/task_test.go (3 hunks)
- go.mod (6 hunks)
- mail/application.go (1 hunks)
- mail/application_test.go (8 hunks)
- mail/service_provider.go (2 hunks)
- queue/application.go (1 hunks)
- queue/config.go (2 hunks)
- queue/config_test.go (4 hunks)
- queue/console/migrate_make_command.go (1 hunks)
- queue/console/migrate_make_command_test.go (1 hunks)
- queue/console/migrate_stubs.go (1 hunks)
- queue/driver.go (1 hunks)
- queue/driver_async.go (1 hunks)
- queue/driver_async_test.go (1 hunks)
- queue/driver_sync.go (1 hunks)
- queue/driver_sync_test.go (1 hunks)
- queue/job.go (1 hunks)
- queue/service_provider.go (1 hunks)
- queue/task.go (2 hunks)
- queue/task_test.go (1 hunks)
- queue/utils_test.go (2 hunks)
- queue/worker.go (1 hunks)
Files skipped from review due to trivial changes (3)
- .github/workflows/pr-check-title.yml
- contracts/queue/task.go
- queue/utils_test.go
Additional comments not posted (102)
contracts/queue/job.go (2)
11-13: Ensure Type Consistency and Proper Usage forArgsFieldChanging
Argsfrom[]Argto[]anyincreases flexibility but may introduce type safety issues. Ensure that all usages and validations handle the[]anytype correctly.
13-13: Confirm the Purpose and Usage ofDelayFieldThe
Delayfield is added to theJobsstruct. Ensure that this field is used correctly throughout the codebase and matches the intended delay functionality.contracts/queue/queue.go (4)
4-4: OptimizeWorkerMethod Parameter TypeThe parameter type for the
Workermethod is changed topayloads ...*Args. This change can improve clarity and flexibility. Ensure that all calls toWorkerhandle the new parameter type correctly.
6-6: Register Method: Ensure Proper Job RegistrationThe
Registermethod now takes a slice ofJobas a parameter. Ensure that this method correctly registers the provided jobs and handles any potential errors.
9-12: New Methods:GetJobs,GetJob, andJobThe
Queueinterface now includesGetJobs,GetJob, andJobmethods. Ensure that these methods are implemented correctly and provide the intended functionality.
19-19: New Method:ShutdownThe
Workerinterface now includes aShutdownmethod. Ensure that this method is implemented correctly and provides a graceful shutdown mechanism for workers.contracts/queue/driver.go (1)
3-16: NewDriverInterface: Ensure Completeness and CorrectnessThe
Driverinterface introduces methods for job queue operations. Ensure that each method is implemented correctly and aligns with the intended functionality. Additionally, confirm that the interface supports all required operations for the queue drivers.event/service_provider.go (1)
6-6: Package Import Alias: Ensure ConsistencyThe package import alias for
event/consoleis changed toeventconsole. Ensure that the new alias is used consistently throughout the file and does not cause any conflicts.queue/task_test.go (2)
30-31: Ensure type safety for job arguments.The
Argsfield now expects[]any, which can lead to type safety issues. Ensure that the arguments are correctly typed and validated.Are there any validations in place to ensure that the arguments passed to the job are of the expected types?
36-42: Good practice: Ensure cleanup after tests.The test ensures that the created file is removed after the test. This is a good practice to avoid side effects in tests.
queue/service_provider.go (1)
35-38: Ensure command registration is clear and maintainable.Registering commands in a separate method helps keep the
Bootmethod clean and maintainable. This is a good practice.queue/driver_sync.go (5)
13-17: LGTM!The
NewSyncmethod correctly initializes theSyncstruct.
19-21: LGTM!The
Connectionmethod correctly returns the connection string.
23-25: LGTM!The
Drivermethod correctly returns the driver type.
44-47: Consider adding error handling.The
Latermethod should handle potential errors fromtime.Sleepand job execution.- time.Sleep(time.Duration(delay) * time.Second) - return job.Handle(args...) + if err := time.Sleep(time.Duration(delay) * time.Second); err != nil { + return err + } + if err := job.Handle(args...); err != nil { + return err + } + return nilLikely invalid or redundant comment.
31-41: Consider adding error handling.The
Bulkmethod should handle potential errors fromtime.Sleepand job execution.+ var sleepErr error for _, job := range jobs { if job.Delay > 0 { - time.Sleep(time.Duration(job.Delay) * time.Second) + if sleepErr = time.Sleep(time.Duration(job.Delay) * time.Second); sleepErr != nil { + return sleepErr + } } if err := job.Job.Handle(job.Args...); err != nil { return err } } return nilLikely invalid or redundant comment.
queue/driver.go (1)
23-28: LGTM!The
NewDriverImplmethod correctly initializes theDriverImplstruct.event/task.go (2)
Line range hint
12-17:
LGTM!The
NewTaskmethod correctly initializes theTaskstruct.
65-71: LGTM!The
eventArgsToQueueArgsmethod correctly converts event arguments to queue arguments.queue/config.go (5)
40-43: LGTM!The method logic is sound.
47-53: LGTM!The method logic is sound.
Line range hint
11-14:
LGTM!The method logic is sound.
Line range hint
18-20:
LGTM!The method logic is sound.
55-58: Verify the usage ofOrmFacade.Ensure that
OrmFacadeis correctly configured and imported.queue/application.go (6)
13-16: LGTM!The method logic is sound.
36-41: LGTM!The method logic is sound.
45-46: LGTM!The method logic is sound.
48-49: LGTM!The method logic is sound.
52-53: LGTM!The method logic is sound.
57-57: LGTM!The method logic is sound.
queue/worker.go (2)
21-28: LGTM!The method logic is sound.
78-80: LGTM!The method logic is sound.
event/task_test.go (4)
Line range hint
34-40:
LGTM!The test logic is sound.
Line range hint
51-57:
LGTM!The test logic is sound.
Line range hint
64-66:
LGTM!The test logic is sound.
Line range hint
73-75:
LGTM!The test logic is sound.
queue/driver_async.go (7)
21-25: LGTM!The
NewASyncmethod correctly initializes theASyncstruct.
27-29: LGTM!The
Connectionmethod correctly returns the connection string.
31-33: LGTM!The
Drivermethod correctly returns the driver type.
35-41: LGTM!The
Pushmethod correctly handles adding a job to the async job queue.
43-62: LGTM!The
Bulkmethod correctly handles adding multiple jobs to the async job queue, including delayed jobs.
64-73: LGTM!The
Latermethod correctly schedules a job to be added to the async job queue after a delay.
75-91: LGTM!The
Popmethod correctly handles retrieving and removing the first job from the async job queue.queue/task.go (7)
17-28: LGTM!The
NewTaskmethod correctly initializes theTaskstruct.
32-39: LGTM!The
NewChainTaskmethod correctly initializes theTaskstruct for a chain of jobs.
43-49: LGTM!The
Delaymethod correctly sets the delay time for the task.
51-66: LGTM!The
Dispatchmethod correctly handles task dispatching, including bulk and delayed dispatching.
Line range hint
70-84:
LGTM!The
DispatchSyncmethod correctly handles synchronous task dispatching.
88-92: LGTM!The
OnConnectionmethod correctly sets the connection name and updates the driver.
97-101: LGTM!The
OnQueuemethod correctly sets the queue name.queue/job.go (5)
31-35: LGTM!The
NewJobImplmethod correctly initializes theJobImplstruct.
37-46: LGTM!The
Registermethod correctly registers jobs to the injector and updates the signatures list.
48-57: LGTM!The
Callmethod correctly handles job invocation using the injector.
59-63: LGTM!The
Getmethod correctly retrieves a job using the injector.
65-79: LGTM!The
GetJobsmethod correctly retrieves all registered jobs using their signatures.queue/console/migrate_make_command.go (7)
20-22: LGTM!The
NewMigrateMakeCommandmethod correctly initializes the struct.
24-26: LGTM!The
Signaturemethod correctly returns the command signature.
29-31: LGTM!The
Descriptionmethod correctly returns the command description.
34-39: LGTM!The
Extendmethod correctly returns the extension details of the console command.
41-53: LGTM!The
Handlemethod correctly handles file creation for migrations and prints a success message.
55-66: LGTM!The
getStubmethod correctly retrieves the stubs for different database drivers.
69-73: LGTM!The
getPathmethod correctly constructs the path for the migration files.queue/console/migrate_stubs.go (4)
5-10: Ensure compatibility with MySQL standards.The
created_atandupdated_atcolumns are defined withDATETIME(3). Ensure this precision is supported in your MySQL version.
41-43: Ensure compatibility with PostgreSQL standards.The
BIGINT GENERATED BY DEFAULT AS IDENTITYsyntax is used for theidcolumn. Verify this syntax is supported in your PostgreSQL version.
74-76: Ensure compatibility with SQLite standards.The
BIGINT PRIMARY KEY AUTOINCREMENTsyntax is used for theidcolumn. Verify this syntax is supported in your SQLite version.
Line range hint
107-109: Ensure compatibility with SQL Server standards.The
BIGINT NOT NULL IDENTITY(1,1)syntax is used for theidcolumn. Verify this syntax is supported in your SQL Server version.queue/driver_sync_test.go (3)
27-38: Ensure mock objects are properly set up.The mock objects
mockConfigandmockQueueare initialized and passed to the application. Ensure all necessary methods are mocked for the tests.
45-54: Ensure all relevant scenarios are covered.The test case covers the basic functionality of the sync queue. Consider adding more scenarios to test edge cases and error handling.
56-77: Ensure all relevant scenarios are covered.The test case covers the basic functionality of the chain sync queue. Consider adding more scenarios to test edge cases and error handling.
queue/config_test.go (6)
Line range hint
38-49: Ensure all relevant scenarios are covered.The test case covers the basic configuration of the queue. Consider adding more scenarios to test edge cases and different configurations.
65-71: Ensure all relevant scenarios are covered.The test case covers the basic configuration of the driver. Consider adding more scenarios to test edge cases and different configurations.
73-78: Ensure all relevant scenarios are covered.The test case covers the basic configuration of the driver when a connection is provided. Consider adding more scenarios to test edge cases and different configurations.
80-86: Ensure all relevant scenarios are covered.The test case covers the basic functionality of the
Viamethod. Consider adding more scenarios to test edge cases and different configurations.
88-93: Ensure all relevant scenarios are covered.The test case covers the basic functionality of the
Viamethod when a connection is provided. Consider adding more scenarios to test edge cases and different configurations.
95-110: Ensure all relevant scenarios are covered.The test case covers the basic functionality of the failed jobs query. Consider adding more scenarios to test edge cases and different configurations.
database/console/migrate_stubs.go (4)
8-9: Ensure compatibility with MySQL standards.The
BIGINT(20) UNSIGNED NOT NULL AUTO_INCREMENTsyntax is used for theidcolumn. Verify this syntax is supported in your MySQL version.
41-43: Ensure compatibility with PostgreSQL standards.The
BIGINT GENERATED BY DEFAULT AS IDENTITYsyntax is used for theidcolumn. Verify this syntax is supported in your PostgreSQL version.
74-76: Ensure compatibility with SQLite standards.The
BIGINT PRIMARY KEY AUTOINCREMENTsyntax is used for theidcolumn. Verify this syntax is supported in your SQLite version.
107-109: Ensure compatibility with SQL Server standards.The
BIGINT NOT NULL IDENTITY(1,1)syntax is used for theidcolumn. Verify this syntax is supported in your SQL Server version.mail/application.go (1)
74-74: Ensure correct job creation.The creation of the job using
r.queue.Jobseems correct. However, verify thatNewSendMailJoband the arguments passed are appropriate and consistent with the rest of the implementation.go.mod (2)
29-29: Dependency addition approved.The addition of
github.com/samber/do/v2is appropriate and follows best practices.
95-95: Indirect dependency addition approved.The addition of the indirect dependency
github.com/mitchellh/hashstructure/v2is appropriate and follows best practices.mail/application_test.go (11)
41-41: Test case addition approved.The test case for sending mail via port 465 is correct and follows best practices.
53-53: Test case addition approved.The test case for sending mail via port 587 is correct and follows best practices.
65-65: Test case addition approved.The test case for sending mail with a "From" address is correct and follows best practices.
78-78: Test case addition approved.The test case for sending mail using a mailable object is correct and follows best practices.
84-84: Test case addition approved.The test case for queuing mail is correct and follows best practices.
86-87: Job registration approved.The registration of the new job for the queue is correct and follows best practices.
90-90: Error handling approved.The error handling statement is correct and follows best practices.
115-115: Test case addition approved.The test case for queuing mail with a mailable object is correct and follows best practices.
117-118: Job registration approved.The registration of the new job for the queue is correct and follows best practices.
121-121: Error handling approved.The error handling statement is correct and follows best practices.
139-142: Mock configuration function approved.The mock configuration function is correct and follows best practices.
queue/driver_async_test.go (9)
1-1: Package declaration approved.The package declaration is correct and follows best practices.
3-15: Import statements approved.The import statements are correct and follow best practices.
17-23: Test job variables approved.The test job variables are correct and follow best practices.
25-30: Test suite struct approved.The test suite struct is correct and follows best practices.
32-51: Test suite setup function approved.The test suite setup function is correct and follows best practices.
53-55: Setup test function approved.The setup test function is correct and follows best practices.
57-81: Test function for default async queue approved.The test function for the default async queue is correct and follows best practices.
83-111: Test function for delay async queue approved.The test function for the delay async queue is correct and follows best practices.
113-141: Test function for custom async queue approved.The test function for the custom async queue is correct and follows best practices.
…queue # Conflicts: # go.mod # go.sum
# Conflicts: # go.mod # go.sum
# Conflicts: # go.mod # go.sum
|
Currently, machinery has resumed maintenance, so this PR may no longer be needed. |
|
machinery still doesn't support the DB driver, right? |
Yes |
|
If so, I think it's better to implement the DB driver ourselves. |
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
ℹ Additional Information
Summary by CodeRabbit
New Features
Driverfor managing job queues with methods for pushing, popping, and scheduling jobs.MigrateMakeCommandstruct for creating migrations for failed queue jobs.Shutdownmethod for theWorkerto allow graceful shutdown.Via()andFailedJobsQuery().Bug Fixes
RegisterandBoot.Refactor
machinerywithdriverinWorkerandTaskstructs for better integration.Tests
Documentation