Update Azure Service Bus components to track2 SDK by ItalyPaleAle · Pull Request #1702 · dapr/components-contrib

ItalyPaleAle · 2022-05-04T04:38:25Z

Description

Move the Azure Service Bus components (binding and pubsub) to the new "track2" SDKs. Thanks a lot to @halspang for the initial work in #1648!

Thanks to the update and some refactoring work, two metadata keys for Azure Service Bus Pub/Sub are not needed anymore:

prefetchCount
maxActiveMessagesRecoveryInSec

Notes:

Because the Service Bus SDKs depended on the newer version of azcore and azidentity, I had to update the other Azure SDKs that were already in "track2". Some of the updates contained breaking changes in the APIs, which I've fixed.
~~This requires Go 1.18 so it depends on Updated to Go 1.18 #1697 (which depends on Use revive instead of golint #1685 itself)~~

Issue reference

Please reference the issue this PR will:
closes #1532
closes #1531
closes #1701

ItalyPaleAle · 2022-05-04T05:52:28Z

@@ -489,7 +487,6 @@ func (a *azureServiceBus) Subscribe(req pubsub.SubscribeRequest, handler pubsub.
 			attempts := readAttemptsStale()
 			if attempts == 0 {
 				a.logger.Errorf("Subscription to topic %s lost connection, unable to recover after %d attempts", sub.topic, a.metadata.MaxReconnectionAttempts)


This is unchanged, but wondering if we should make the app crash here (Fatalf)? Otherwise, the error would get logged and the app continues to work but without a connection to Service Bus.

ItalyPaleAle · 2022-05-07T23:09:34Z

-	}
-}
-
-func TestNewPubsubMessageFromAzServiceBusMessage(t *testing.T) {


This test can't be implemented with track2 SDKs. The method to create these objects is un-exported.

Can be re-added if Azure/azure-sdk-for-go#17848 is merged

Signed-off-by: Alessandro (Ale) Segala <[email protected]> Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

Includes some minor refactoring of auth code Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

Co-authored-by: halspang <[email protected]> Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

) * certification test for eventhubs binding Signed-off-by: tanvigour <[email protected]> * modified go.mod and go.sum Signed-off-by: tanvigour <[email protected]> * Add connection string testing Signed-off-by: tanvigour <[email protected]> * iothub testing Signed-off-by: tanvigour <[email protected]> * address feedback and run test Signed-off-by: tanvigour <[email protected]> * Install Azure CLI IOT hub extension Signed-off-by: Bernd Verst <[email protected]> * make modtidy-all Signed-off-by: Bernd Verst <[email protected]> * covering all eventhubs test cases Signed-off-by: tanvigour <[email protected]> * dependency changes after go modtidy-all Signed-off-by: tanvigour <[email protected]> Co-authored-by: Bernd Verst <[email protected]> Co-authored-by: Yaron Schneider <[email protected]> Co-authored-by: Looong Dai <[email protected]> Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

Signed-off-by: pigletfly <[email protected]> Co-authored-by: Yaron Schneider <[email protected]> Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

* Updated to Go 1.18 Signed-off-by: Alessandro (Ale) Segala <[email protected]> * Added go.work file With Go 1.18, this allows gopls (the Go language server used for example in VS Code) to work inside test apps too. See: https://go.dev/doc/tutorial/workspaces Signed-off-by: ItalyPaleAle <[email protected]> Signed-off-by: ItalyPaleAle <[email protected]> * Removed go.work Signed-off-by: ItalyPaleAle <[email protected]> * 💄 Signed-off-by: ItalyPaleAle <[email protected]> Co-authored-by: Bernd Verst <[email protected]> Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

Signed-off-by: ItalyPaleAle <[email protected]> Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

* Add metadata property to configure BatchingMaxSize&batchingMaxMessages in Pulsar Signed-off-by: saberwang <[email protected]> * sort field Signed-off-by: saberwang <[email protected]> * [pubsub]fix unit test bug Signed-off-by: saberwang <[email protected]> * remove unrelated changes Signed-off-by: saberwang <[email protected]> * Delete hard coded Metadata Signed-off-by: saberwang <[email protected]> * remove .history Signed-off-by: saberwang <[email protected]> * restore .gitignore Signed-off-by: saberwang <[email protected]> * Hard coding default values and adding 'BatchingMaxPublishDelay' metadata Signed-off-by: saberwang <[email protected]> * fix code format Signed-off-by: saberwang <[email protected]> * formatting code Signed-off-by: saberwang <[email protected]> Co-authored-by: Looong Dai <[email protected]> Co-authored-by: Bernd Verst <[email protected]> Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

The methods to create a message with a body are not exported Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

…nto service-bus-track2

Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

Signed-off-by: ItalyPaleAle <[email protected]>

berndverst · 2022-05-12T21:26:49Z

/ok-to-test

berndverst · 2022-05-12T21:30:59Z

@ItalyPaleAle please fix linter issues. Also use /ok-to-test to trigger actions for conformance and certification tests (separately from this PR) to run the Service Bus Queues binding certification test.

This greatly simplifies certain parts of the code, reducing the number of goroutines and likely improving performance. Performance for end-users improves too as there's no need anymore to pause for 2 seconds every time that we reach `maxActiveMessages`. Additionally, with this change the config options `prefetchCount` and `maxActiveMessagesRecoveryInSec` are removed as unnecessary anymore. Signed-off-by: ItalyPaleAle <[email protected]>

berndverst · 2022-05-12T22:17:06Z

At the moment this breaks EventHubs Binding, PubSub and Service Bus Queues Binding.

Please take a look by running the certification and conformance tests via /ok-to-test

Signed-off-by: ItalyPaleAle <[email protected]>

ItalyPaleAle · 2022-05-13T00:35:43Z

/ok-to-test

ItalyPaleAle · 2022-05-13T00:42:03Z

@berndverst This is ready for review IMHO.

All tests are passing (waiting for the conformance tests to complete)
I have confirmed that both the binding and pubsub are able to recover successfully in case of failures (both pubsub and input/output bindings)

Once this is merged, I'll need to update the docs because of metadata changes, as well as our E2E tests if needed.

berndverst · 2022-05-13T01:26:56Z

Eventhubs Binding conformance test is failing
Eventhubs Binding certification test is failing
Service Bus Queues Binding certification test is failing

(you can see in the results of your /ok-to-test run)

Signed-off-by: ItalyPaleAle <[email protected]>

ItalyPaleAle · 2022-05-13T01:47:22Z

/ok-to-test

ItalyPaleAle · 2022-05-13T01:48:27Z

I'll look into the Service Bus binding failure tomorrow.

For Event Hubs, that's very odd because I did not touch that module besides for a minimal change that was required. However, it looked like the SDK got upgraded (which was un-intentional). I've reverted the change to the SDK and let's see if that fixes the tests.

This reverts commit 212220a. Signed-off-by: ItalyPaleAle <[email protected]>

Signed-off-by: ItalyPaleAle <[email protected]>

…ng cert test Signed-off-by: ItalyPaleAle <[email protected]>

In case of a failure in the handler (ie. users' code), the message should be correctly re-enqueued, which means that messages will be re-delivered later and won't be in order. Signed-off-by: ItalyPaleAle <[email protected]>

ItalyPaleAle · 2022-05-13T21:20:51Z

@berndverst I have fixed all tests that were failing because of changes in my code.

After fixing dependencies, there were 2 tests that fail.

Service Bus Queue binding: when a message fails to be processed by a handler, it's inserted back into the queue. This means that it's re-delivered later, so it may not come in order. However the conf tests were expecting messages to be in order. I think this was a problem with the tests as the re-enqueueing of the message is arguably the desired behavior
Event Hubs binding: this is still failing and it looks like it was failing before 100% of the times when running on master. See: https://github.com/dapr/components-contrib/actions/workflows/certification.yml The error in this PR is the same. The test is broken and needs to be fixed, but it should not block this PR.

(On why the Event Hubs test is failing: the test TestAzureServiceBusQueueRetriesOnError is creating 2 Event Hubs listeners with the same consumer group, which seems to be disallowed)

Test runs:
https://github.com/dapr/components-contrib/actions/runs/2321897134
https://github.com/dapr/components-contrib/actions/runs/2321897110

codecov · 2022-05-13T23:39:26Z

Codecov Report

Merging #1702 (c8c39e7) into master (bfd87eb) will decrease coverage by 0.12%.
The diff coverage is 28.57%.

@@            Coverage Diff             @@
##           master    #1702      +/-   ##
==========================================
- Coverage   36.37%   36.24%   -0.13%     
==========================================
  Files         166      173       +7     
  Lines       15488    15557      +69     
==========================================
+ Hits         5633     5639       +6     
- Misses       9228     9287      +59     
- Partials      627      631       +4

Impacted Files	Coverage Δ
authentication/azure/auth.go	`38.12% <0.00%> (-1.77%)`	⬇️
authentication/azure/auth_amqp.go	`0.00% <0.00%> (ø)`
bindings/azure/cosmosdb/cosmosdb.go	`18.00% <ø> (ø)`
bindings/azure/eventhubs/eventhubs.go	`14.79% <0.00%> (ø)`
bindings/cron/cron.go	`87.23% <ø> (ø)`
bindings/http/http.go	`87.87% <ø> (ø)`
bindings/kubernetes/kubernetes.go	`17.14% <ø> (ø)`
bindings/localstorage/localstorage.go	`4.95% <0.00%> (-0.17%)`	⬇️
internal/component/kafka/consumer.go	`0.00% <0.00%> (ø)`
internal/component/kafka/kafka.go	`0.00% <0.00%> (ø)`
... and 28 more

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 785ed60...c8c39e7. Read the comment docs.

ItalyPaleAle requested review from a team as code owners May 4, 2022 04:38

ItalyPaleAle mentioned this pull request May 4, 2022

WIP: Move Azure Service Bus to track2 sdk #1648

Closed

3 tasks

ItalyPaleAle force-pushed the service-bus-track2 branch from cadd5ab to bbb776e Compare May 4, 2022 04:44

ItalyPaleAle commented May 4, 2022

View reviewed changes

ItalyPaleAle marked this pull request as draft May 4, 2022 05:52

ItalyPaleAle changed the title ~~WIP: Update Azure Service Bus components to track2 SDK~~ Update Azure Service Bus components to track2 SDK May 4, 2022

ItalyPaleAle mentioned this pull request May 4, 2022

Publish to service bus fails with i/o timeout after a couple of ms and requires pod restart dapr/dapr#4416

Closed

ItalyPaleAle marked this pull request as ready for review May 6, 2022 20:51

ItalyPaleAle commented May 7, 2022

View reviewed changes

ItalyPaleAle force-pushed the service-bus-track2 branch from fba3884 to 8d97718 Compare May 7, 2022 23:15

ItalyPaleAle and others added 9 commits May 7, 2022 16:16

Updated to Go 1.18

39996e4

Signed-off-by: Alessandro (Ale) Segala <[email protected]> Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

Updated Azure SDKs that are on track2

8e58f17

Includes some minor refactoring of auth code Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

Updated Service Bus components to track2 SDK

b4581b0

Co-authored-by: halspang <[email protected]> Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

Use revive instead of golint (dapr#1685)

021aea7

Signed-off-by: pigletfly <[email protected]> Co-authored-by: Yaron Schneider <[email protected]> Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

💄 & 🧹

cb4f15f

Signed-off-by: ItalyPaleAle <[email protected]> Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

This test can't work with track2 SDKs

957bb8d

The methods to create a message with a body are not exported Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

ItalyPaleAle force-pushed the service-bus-track2 branch from 8d97718 to 957bb8d Compare May 7, 2022 23:16

ItalyPaleAle added 2 commits May 7, 2022 16:20

Merge branch 'master' of https://github.com/dapr/components-contrib i…

dfd3654

…nto service-bus-track2

🧹

9d3cec7

Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

ItalyPaleAle mentioned this pull request May 11, 2022

Service Bus: export NewReceivedMessage Azure/azure-sdk-for-go#17848

Closed

5 tasks

ItalyPaleAle added 2 commits May 12, 2022 09:16

Merge branch 'master' into service-bus-track2

f6bf1d6

Signed-off-by: ItalyPaleAle <[email protected]>

There's such thing as too much logging

5e146fe

Signed-off-by: ItalyPaleAle <[email protected]>

ItalyPaleAle added 6 commits May 12, 2022 15:18

💄

f2020f4

Signed-off-by: ItalyPaleAle <[email protected]>

Fixed pubsub tests

9161de1

Signed-off-by: ItalyPaleAle <[email protected]>

These packages should have never been upgraded

8a61deb

Signed-off-by: ItalyPaleAle <[email protected]>

Ensuring we don't fetch 1 message more than max active

1151f1f

Signed-off-by: ItalyPaleAle <[email protected]>

Adding configurable timeout for servicebusqueues operations

f89e345

Signed-off-by: ItalyPaleAle <[email protected]>

Persistent connection for invoking SB queues

2db5395

Signed-off-by: ItalyPaleAle <[email protected]>

berndverst added the do-not-merge PR is not ready for merging label May 13, 2022

Reverted Event Hub SDK update

212220a

Signed-off-by: ItalyPaleAle <[email protected]>

ItalyPaleAle added 2 commits May 13, 2022 19:53

Revert "Reverted Event Hub SDK update"

75e85a7

This reverts commit 212220a. Signed-off-by: ItalyPaleAle <[email protected]>

Fix Azure deploy for users with a first.last email

3bf733d

Signed-off-by: ItalyPaleAle <[email protected]>

ItalyPaleAle force-pushed the service-bus-track2 branch from 98f0043 to 3bf733d Compare May 13, 2022 19:53

Added some sleep that should help reduce flakiness in eventhubs bindi…

86d5798

…ng cert test Signed-off-by: ItalyPaleAle <[email protected]>

ItalyPaleAle force-pushed the service-bus-track2 branch from 191f725 to 86d5798 Compare May 13, 2022 20:56

Changed servicebusqueue cert test

b492a42

In case of a failure in the handler (ie. users' code), the message should be correctly re-enqueued, which means that messages will be re-delivered later and won't be in order. Signed-off-by: ItalyPaleAle <[email protected]>

Merge branch 'master' into service-bus-track2

c8c39e7

berndverst approved these changes May 13, 2022

View reviewed changes

berndverst merged commit e5e9011 into dapr:master May 13, 2022

berndverst mentioned this pull request May 13, 2022

Fix Azure deploy script for users with a first.last email #1725

Closed

ItalyPaleAle mentioned this pull request May 19, 2022

Update Azure Service Bus docs for 1.8 dapr/docs#2449

Closed

berndverst mentioned this pull request May 26, 2022

REQUEST: Approver Status for @italypaleale in Contrib dapr/community#177

Closed

halspang mentioned this pull request May 31, 2022

Input binding with Azure Service Bus and session queue and .NET SDK dapr/dapr#4604

Open

berndverst removed the do-not-merge PR is not ready for merging label Jun 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update Azure Service Bus components to track2 SDK#1702

Update Azure Service Bus components to track2 SDK#1702
berndverst merged 26 commits into
dapr:masterfrom
ItalyPaleAle:service-bus-track2

ItalyPaleAle commented May 4, 2022 •

edited

Loading

Uh oh!

ItalyPaleAle May 4, 2022

Uh oh!

ItalyPaleAle May 7, 2022

Uh oh!

berndverst commented May 12, 2022

Uh oh!

berndverst commented May 12, 2022

Uh oh!

berndverst commented May 12, 2022

Uh oh!

ItalyPaleAle commented May 13, 2022

Uh oh!

ItalyPaleAle commented May 13, 2022 •

edited

Loading

Uh oh!

berndverst commented May 13, 2022

Uh oh!

ItalyPaleAle commented May 13, 2022

Uh oh!

ItalyPaleAle commented May 13, 2022

Uh oh!

ItalyPaleAle commented May 13, 2022 •

edited

Loading

Uh oh!

codecov Bot commented May 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

ItalyPaleAle commented May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue reference

Uh oh!

ItalyPaleAle May 4, 2022

Choose a reason for hiding this comment

Uh oh!

ItalyPaleAle May 7, 2022

Choose a reason for hiding this comment

Uh oh!

berndverst commented May 12, 2022

Uh oh!

berndverst commented May 12, 2022

Uh oh!

berndverst commented May 12, 2022

Uh oh!

ItalyPaleAle commented May 13, 2022

Uh oh!

ItalyPaleAle commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

berndverst commented May 13, 2022

Uh oh!

ItalyPaleAle commented May 13, 2022

Uh oh!

ItalyPaleAle commented May 13, 2022

Uh oh!

ItalyPaleAle commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented May 13, 2022

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ItalyPaleAle commented May 4, 2022 •

edited

Loading

ItalyPaleAle commented May 13, 2022 •

edited

Loading

ItalyPaleAle commented May 13, 2022 •

edited

Loading