feat: optimize queue module by hwbrzzl · Pull Request #987 · goravel/framework

hwbrzzl · 2025-04-02T12:04:30Z

📑 Description

✅ Checks

Added test cases for my code

github-actions

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.

Benchmark suite	Current: `c22b13c`	Previous: `1fa2c00`	Ratio
`Benchmark_EncryptString`	`5144` ns/op 2152 B/op 14 allocs/op	`1603` ns/op 2152 B/op 14 allocs/op	`3.21`
`Benchmark_EncryptString - ns/op`	`5144` ns/op	`1603` ns/op	`3.21`
`Benchmark_DecryptString`	`6680` ns/op 2032 B/op 16 allocs/op	`2110` ns/op 2032 B/op 16 allocs/op	`3.17`
`Benchmark_DecryptString - ns/op`	`6680` ns/op	`2110` ns/op	`3.17`

This comment was automatically generated by workflow using github-action-benchmark.

codecov · 2025-04-03T09:50:07Z

Codecov Report

Attention: Patch coverage is 54.94223% with 351 lines in your changes missing coverage. Please review.

Project coverage is 70.19%. Comparing base (1fa2c00) to head (c22b13c).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
queue/driver_machinery_log.go	30.70%	88 Missing ⚠️
queue/utils.go	55.00%	59 Missing and 4 partials ⚠️
queue/worker.go	47.67%	40 Missing and 5 partials ⚠️
mail/application.go	0.00%	37 Missing ⚠️
support/convert/type.go	61.62%	24 Missing and 9 partials ⚠️
queue/driver_machinery.go	64.44%	22 Missing and 10 partials ⚠️
queue/application.go	26.08%	17 Missing ⚠️
queue/service_provider.go	0.00%	12 Missing ⚠️
queue/pending_job.go	92.40%	4 Missing and 2 partials ⚠️
queue/driver_sync.go	80.76%	5 Missing ⚠️
... and 5 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #987      +/-   ##
==========================================
- Coverage   71.10%   70.19%   -0.92%     
==========================================
  Files         172      176       +4     
  Lines       11833    12376     +543     
==========================================
+ Hits         8414     8687     +273     
- Misses       3064     3310     +246     
- Partials      355      379      +24

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull Request Overview

This PR aims to optimize the queue module by updating job creation methods, argument types, and associated error messages while standardizing JSON instantiation. Key changes include:

Refactoring job argument types from []any to []Arg and updating related interfaces.
Renaming json.NewJson() to json.New() across application and test files.
Updating error messages and refining API signatures in the queue contracts.

Reviewed Changes

Copilot reviewed 68 out of 69 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
mail/application.go	Updated job creation to use a structured slice of arguments instead of generic []any.
log/, http/, foundation/, crypt/	Standardized JSON initialization following the renaming from NewJson() to New().
event/task*.go	Updated task dispatch processing to use new argument types and updated queue interface usage.
errors/list.go	Revised error messages and added new error constants for various queue-related errors.
contracts/queue/*	Updated API design in the queue contracts including method signatures and type changes.

Files not reviewed (1)

go.mod: Language not supported

Comments suppressed due to low confidence (1)

errors/list.go:128

[nitpick] The error message 'the driver of queue %s is sync, not need to run' has awkward phrasing. Consider rephrasing it to something like 'the driver of queue %s is synchronous and does not require running'.

QueueDriverSyncNotNeedToRun    = New("the driver of queue %s is sync, not need to run")

contracts/queue/queue.go

krishankumar01

Added a few minor points

queue/driver_machinery.go

krishankumar01 · 2025-05-04T09:05:39Z

mail/application.go

+		},
+		{
+			Type:  "[]string",
+			Value: convertMapHeadersToSlice(r.headers),


Why are we converting Headers to a slice here? Since SendMailJob converts it back to a map[string]string, this seems like an unnecessary round-trip.

Queue doesn't support map[string]string for now. We can implement it in another PR.

krishankumar01 · 2025-05-04T09:06:21Z

mail/job.go

 	}

-	return SendMail(r.config, from, subject, body, recipient, cc, bcc, replyTo, attachments, headers)
+	return SendMail(r.config, from, subject, body, recipient, cc, bcc, replyTo, attachments, convertSliceHeadersToMap(headers))


Here, we are converting it back to the map

krishankumar01 · 2025-05-04T09:19:47Z

queue/job_repository.go

-	Exception  string          // The exception that caused the job to fail.
-	FailedAt   carbon.DateTime // The timestamp when the job failed.
+	ID         uint            `gorm:"primaryKey" db:"id"`
+	UUID       string          `db:"uuid"`


If this is related to job's id, we can rename it to JobId since UUID doesn't convey any information specific to the column.

If this is related to job's id, we can rename it to JobId since UUID doesn't convey any information specific to the column.

Laravel use UUID, see https://github.com/laravel/framework/blob/12.x/src/Illuminate/Queue/Console/stubs/failed_jobs.stub

UUID is fine, the same with Laravel.

krishankumar01 · 2025-05-04T09:33:39Z

queue/utils.go

+
+type Task struct {
+	Job
+	Uuid  string `json:"uuid"`


We can rename it TaskId.

krishankumar01 · 2025-05-04T09:36:12Z

queue/utils.go

+	return payload, nil
+}
+
+func JsonToTask(payload string, queue contractsqueue.Queue, json foundation.Json) (contractsqueue.Task, error) {


We can remove this if it's not being used.

It's used by the Redis driver, and will be used by the DB driver.

krishankumar01 · 2025-05-04T09:37:45Z

queue/worker.go

+		}
+	}
+
+	// TODO: print success log


Good catch, I vaguely remember missing something.

krishankumar01 · 2025-05-04T09:37:54Z

queue/worker.go

+				r.log.Error(errors.QueueFailedToSaveFailedJob.Args(err, job))
 			}
+
+			// TODO: print failed log


queue/utils.go

almas-x · 2025-05-08T07:37:16Z

contracts/queue/config.go

+	Config() config.Config
 	Debug() bool
-	DefaultConnection() string
+	Default() (connection, queue string, concurrent int)


In Go's standard library, it's highly uncommon to see functions with three return values. Typically, the second return value is designated for either an error or a bool, adhering to Go's simplicity and clarity principles. Considering this, the Default method could be split into multiple methods to better align with Go conventions and improve code readability.

Good point, will optimize it in another PR.

github-actions bot reviewed Apr 3, 2025

View reviewed changes

hwbrzzl force-pushed the bowen/optimize-queue branch from d7a3cd3 to fb29765 Compare April 13, 2025 09:55

hwbrzzl closed this Apr 13, 2025

hwbrzzl reopened this Apr 13, 2025

hwbrzzl force-pushed the bowen/optimize-queue branch 2 times, most recently from f5908be to 5c11a57 Compare April 18, 2025 11:39

hwbrzzl force-pushed the bowen/optimize-queue branch from 61ffc58 to c176cb4 Compare April 24, 2025 08:52

hwbrzzl force-pushed the bowen/optimize-queue branch from f5c23b2 to b9e1411 Compare May 3, 2025 14:45

hwbrzzl marked this pull request as ready for review May 4, 2025 02:06

hwbrzzl requested a review from a team as a code owner May 4, 2025 02:06

hwbrzzl added the 🚀 Review Ready label May 4, 2025

hwbrzzl requested a review from Copilot May 4, 2025 02:07

Copilot AI reviewed May 4, 2025

View reviewed changes

contracts/queue/queue.go Show resolved Hide resolved

krishankumar01 reviewed May 4, 2025

View reviewed changes

devhaozi reviewed May 4, 2025

View reviewed changes

queue/utils.go Outdated Show resolved Hide resolved

devhaozi reviewed May 4, 2025

View reviewed changes

queue/utils.go Show resolved Hide resolved

devhaozi reviewed May 4, 2025

View reviewed changes

queue/utils.go Outdated Show resolved Hide resolved

devhaozi reviewed May 5, 2025

View reviewed changes

queue/utils.go Outdated Show resolved Hide resolved

hwbrzzl added 11 commits May 8, 2025 14:48

optimize test

e9d9305

optimize

5efc3e4

optimize

80a5b38

remove queue async driver

0bf25c2

optimize test

397a53b

optimize test

9ba42e5

add machinery

67cfe33

add machinery

da306b5

add machinery

cef7036

optimize config

1c4bc90

optimize

f42b1fb

hwbrzzl and others added 23 commits May 8, 2025 14:49

add tests

9fef2d8

recover queue arg

39978fd

optimize

fec8e5c

optimize

1bcc91f

optimize

6160d7f

optimize

79cdfc7

optimize

b4d8ca9

rebase

27519ac

chore: update mocks

a41b64e

optimize

7dfad9b

fix

4e21c95

update mod

49300bd

fix test

dde9fe9

add tests

6474484

add test

8590a19

add helper methods

c6034e5

rebase

af64cd6

fix test

8a8df3d

optimize

7b2d39f

optimize

9307e75

optimize

492eec4

address comments

cc40412

rebase

6fbcc56

hwbrzzl force-pushed the bowen/optimize-queue branch from c83afb9 to 6fbcc56 Compare May 8, 2025 06:53

use MarshalString

c22b13c

hwbrzzl merged commit be9dbad into master May 8, 2025
11 of 14 checks passed

hwbrzzl deleted the bowen/optimize-queue branch May 8, 2025 07:22

almas-x reviewed May 8, 2025

View reviewed changes

Conversation

hwbrzzl commented Apr 2, 2025

📑 Description

✅ Checks

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

codecov bot commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

krishankumar01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions bot left a comment •

edited

Loading

codecov bot commented Apr 3, 2025 •

edited

Loading