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

reject a faulty delivery logic #147

Merged
merged 3 commits into from
Jun 23, 2023
Merged

reject a faulty delivery logic #147

merged 3 commits into from
Jun 23, 2023

Conversation

mariaefi29
Copy link
Contributor

Messages with broken headers (for example, without a separator) has never been rejected and stayed forever in unack.
This PR fixes the bug. If there is an error creating a delivery, we will reject it.

@github-actions
Copy link

github-actions bot commented Jun 22, 2023

Lines Of Code

Language Files Lines Code Comments Blanks Complexity Bytes
Go 29 2938 (-15) 2151 (-14) 284 (+1) 503 (-2) 397 (+2) 77.4K (-40B)
Go (test) 7 1555 (+40) 1248 (+30) 48 (+2) 259 (+8) 87 (+2) 44.3K (+1.1K)

@github-actions
Copy link

Go API Changes

# summary
Inferred base version: v5.1.2
Suggested version: v5.1.3

eventuallyUnacked(t, queue, 0)
eventuallyRejected(t, queue, 6)

require.Len(t, consumer.Deliveries(), 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if it's correct. I want to figure out why we have no deliveries. They should be rejected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I double-checked. This is an expected behaviour, we don't push those deliveries into the delivery channel.

@github-actions
Copy link

github-actions bot commented Jun 22, 2023

Unit Test Coverage

total: (statements) 71.8%
changed lines: (statements) 90.0%

Coverage of changed lines
File Function Coverage
Total 90.0%
queue.go 90.0%
queue.go:186 consumeBatch 100.0%
queue.go:230 newDelivery 96.8%
Coverage diff with base branch
File Function Base Coverage Current Coverage
Total 71.7% 71.8% (+0.1%)
github.com/adjust/rmq/v5/delivery.go newDelivery 80.0% no function
github.com/adjust/rmq/v5/queue.go batchTimeout 92.3% 76.9% (-15.4%)
github.com/adjust/rmq/v5/queue.go consume 77.8% 100.0% (+22.2%)
github.com/adjust/rmq/v5/queue.go consumeBatch 85.7% 90.5% (+4.8%)
github.com/adjust/rmq/v5/queue.go consumerBatchConsume 93.8% 87.5% (-6.3%)
github.com/adjust/rmq/v5/queue.go newDelivery 100.0% 88.9% (-11.1%)

@github-actions
Copy link

github-actions bot commented Jun 22, 2023

Benchmark Result

Benchmark diff with base branch
name     old time/op    new time/op    delta
Queue-2     180µs ± 6%     177µs ± 4%    ~     (p=0.485 n=6+6)

name     old alloc/op   new alloc/op   delta
Queue-2    45.5kB ± 4%    42.7kB ± 7%  -6.00%  (p=0.041 n=6+6)

name     old allocs/op  new allocs/op  delta
Queue-2      91.0 ± 0%      91.0 ± 0%    ~     (all equal)
Benchmark result
name     time/op
Queue-2   177µs ± 4%

name     alloc/op
Queue-2  42.7kB ± 7%

name     allocs/op
Queue-2    91.0 ± 0%

@mariaefi29 mariaefi29 requested a review from wellle June 23, 2023 06:54
@mariaefi29 mariaefi29 added the bug label Jun 23, 2023
queue.go Outdated
}

rejectErr := rd.Reject()
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if err != nil {
if rejectErr != nil {

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, thank you! It was a bug!

@mariaefi29 mariaefi29 merged commit 52c05b0 into master Jun 23, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants