Skip to content

Conversation

@pkedy
Copy link
Member

@pkedy pkedy commented Jul 23, 2021

Description

This fixes the handling of amqp:link:detach-forced errors from Azure Service Bus. While the log message can be suppressed, this type of error should not be ignored because it prevents the component from reconnecting. Reconnecting is the desired behavior when the link is force detached.

Issue reference

Resolves dapr/dapr#3468

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

mthmulders and others added 3 commits July 18, 2021 18:44
The pubsub/nats component was replaced by pubsub/natsstreaming as part
of dapr/dapr#2003, but the corresponding code
in dapr/components-contrib was not removed, so this change removes it.
@pkedy pkedy requested review from a team as code owners July 23, 2021 20:27
Copy link
Member Author

@pkedy pkedy left a comment

Choose a reason for hiding this comment

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

Comments for reviewers

a.logger.Error(innerErr)
var detachError *amqp.DetachError
var ampqError *amqp.Error
if errors.Is(innerErr, detachError) ||
Copy link
Member Author

Choose a reason for hiding this comment

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

*amqp.Error is the most likely terror received, but I'm also detecting *amqp.DetachError in case.

}

return fmt.Errorf("%s error receiving message on topic %s, %s", errorMessagePrefix, s.topic, err)
return fmt.Errorf("%s error receiving message on topic %s, %w", errorMessagePrefix, s.topic, err)
Copy link
Member Author

Choose a reason for hiding this comment

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

Using %w is required to wrap the underlying AMQP error so it can be detected by errors.Is/errors.As.

https://blog.golang.org/go1.13-errors

@artursouza
Copy link
Contributor

Please, change it to the release branch. We merge release into master.

@codecov
Copy link

codecov bot commented Jul 23, 2021

Codecov Report

Merging #1030 (7a991cd) into master (80f0110) will increase coverage by 0.18%.
The diff coverage is 51.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1030      +/-   ##
==========================================
+ Coverage   34.03%   34.21%   +0.18%     
==========================================
  Files         133      132       -1     
  Lines       10750    10769      +19     
==========================================
+ Hits         3659     3685      +26     
+ Misses       6710     6699      -11     
- Partials      381      385       +4     
Impacted Files Coverage Δ
bindings/smtp/smtp.go 47.69% <0.00%> (ø)
pubsub/azure/servicebus/servicebus.go 30.49% <0.00%> (-0.62%) ⬇️
pubsub/azure/servicebus/subscription.go 0.00% <0.00%> (ø)
state/memcached/memcached.go 38.23% <50.00%> (+12.78%) ⬆️
state/cassandra/cassandra.go 25.97% <58.33%> (+2.73%) ⬆️
state/redis/redis.go 43.03% <68.00%> (+4.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f58e0ca...7a991cd. Read the comment docs.

@pkedy pkedy changed the base branch from master to release-1.3 July 23, 2021 20:51
@pkedy pkedy merged commit 54840c2 into dapr:release-1.3 Jul 23, 2021
@pkedy pkedy deleted the fix_asb_force_detach_handling branch July 23, 2021 21:01
pkedy added a commit that referenced this pull request Jul 23, 2021
pkedy added a commit that referenced this pull request Jul 23, 2021
dapr-bot pushed a commit that referenced this pull request Aug 6, 2021
* Fixing the handling of detach errors (#1030)

* Improve error message in case of missing property (#1012)

Co-authored-by: Artur Souza <[email protected]>

* Remove vestigial pubsub/nats code (#1024)

The pubsub/nats component was replaced by pubsub/natsstreaming as part
of dapr/dapr#2003, but the corresponding code
in dapr/components-contrib was not removed, so this change removes it.

* Fixing the handling of detach errors

Co-authored-by: Maarten Mulders <[email protected]>
Co-authored-by: Artur Souza <[email protected]>
Co-authored-by: Simon Leet <[email protected]>

* Revert "Fixing the handling of detach errors (#1030)" (#1031)

This reverts commit 54840c2.

* Fixing the handling of detach errors (#1032)

Co-authored-by: Maarten Mulders <[email protected]>
Co-authored-by: Artur Souza <[email protected]>
Co-authored-by: Simon Leet <[email protected]>
amimimor pushed a commit to amimimor/components-contrib that referenced this pull request Dec 9, 2021
* Fixing the handling of detach errors (dapr#1030)

* Improve error message in case of missing property (dapr#1012)

Co-authored-by: Artur Souza <[email protected]>

* Remove vestigial pubsub/nats code (dapr#1024)

The pubsub/nats component was replaced by pubsub/natsstreaming as part
of dapr/dapr#2003, but the corresponding code
in dapr/components-contrib was not removed, so this change removes it.

* Fixing the handling of detach errors

Co-authored-by: Maarten Mulders <[email protected]>
Co-authored-by: Artur Souza <[email protected]>
Co-authored-by: Simon Leet <[email protected]>

* Revert "Fixing the handling of detach errors (dapr#1030)" (dapr#1031)

This reverts commit 54840c2.

* Fixing the handling of detach errors (dapr#1032)

Co-authored-by: Maarten Mulders <[email protected]>
Co-authored-by: Artur Souza <[email protected]>
Co-authored-by: Simon Leet <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

high cpu usage of dapr sidecars after upgrading to runtime 1.3.0-rc.x

5 participants