updated service bus with new microsoft.azure.servicebus dotnet library #1

shahbarkha · 2017-08-14T17:04:37Z

First cut that allows the foundatio.azureservicebus to use the latest azure service bus dotnet library ( 1.0.0)
Added 2 sample apps for topics and queues

…y version to 1.0.0 from 0.0.7 preview

CLAassistant · 2017-08-14T17:04:44Z

All committers have signed the CLA.

…oreapp, and added few new options to the azuremessagebusoptions.

…e user does not provide any subscription name then by default guid is generated and topic is used with fanout mode

… library. putting the comment for the github issue.

…sample examples.

…onasync, completeasync on different objects bases on the push or pull strategy.

…exception. We instead call the overloaded method with no timeout in that case.

SeanFeldman

@shahbarkha hope it helps

SeanFeldman · 2017-09-12T21:25:36Z

samples/Foundatio.ReceiverConsole/Program.cs

+
+
+            try {
+                var o = new Receiver();


We can now do static async Task MainAsync and remove the Getawaiter().GetResult(). As this was added in the latest .net core..

You can also async Task Main() 🙂

Good tip Blake. I just realized i have removed the reference of sample project from the solution but didnt delete the sample files from the repo. Will be deleting all the samples. I think our unit test cases are great enough to test all the possible scenarios

SeanFeldman · 2017-09-12T21:28:22Z

samples/Foundatio.ReceiverConsole/Properties/AssemblyInfo.cs

+using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
+
+// General Information about an assembly is controlled through the following


remove commented out lines?

We should be able to drop all assembly info's as well or at least just provide a title and description as we do in the Foundatio Project (logging branch)

SeanFeldman · 2017-09-12T21:34:48Z

samples/Foundatio.ReceiverConsole/App.config

@@ -0,0 +1,26 @@
+<?xml version="1.0" encoding="utf-8"?>


You probably don't need this file.

We should be able to remove the app.config completely in all projects

I have actually deleted the sample project itself. I didn't find any value to that when we have good amount of unit test cases covering all the test cases.

SeanFeldman · 2017-09-12T21:35:16Z

samples/Foundatio.ReceiverConsole/App.config

+        <supportedRuntime version="v4.0" sku=".NETFramework,Version=v4.6" />
+    </startup>
+  <runtime>
+    <assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">


Can be replaced entirely with

<AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects> <GenerateBindingRedirectsOutputType>true</GenerateBindingRedirectsOutputType>

SeanFeldman · 2017-09-12T21:38:01Z

samples/Foundatio.ReceiverConsole/packages.config

@@ -0,0 +1,83 @@
+<?xml version="1.0" encoding="utf-8"?>
+<packages>


Can be entirely replaced by PackageReference in csproj.
The benefit is you can reduce number of transitive packages listed as dependencies.
E.g.: xunit has a dependency on xunit.assert and xunit.core which do not need to be referenced if using PackageReference

If we switch to the new project system I don't think we can remove most of these references. We can do this in a second pr as well.

SeanFeldman · 2017-09-12T21:53:20Z

src/Foundatio.AzureServiceBus/Messaging/AzureServiceBusMessageBus.cs

-        private async Task OnMessageAsync(BrokeredMessage brokeredMessage) {
+        // Use this Handler to look at the exceptions received on the MessagePump
+        private Task OnExceptionAsync(ExceptionReceivedEventArgs args) {
+            _logger.Warn(args.Exception, "Message handler encountered an exception.");


Do not swallow args. It contains valuable information valuable information such as

action that failed

entity path

namespace
It all should be logged imo

+1, We upgraded to the new Microsoft logging in foundatio 6. I can pair on this if you'd like

SeanFeldman · 2017-09-12T21:54:56Z

src/Foundatio.AzureServiceBus/Messaging/AzureServiceBusMessageBus.cs

            if (_subscribers.IsEmpty)
                return;

-            _logger.Trace("OnMessageAsync({messageId})", brokeredMessage.MessageId);
+            _logger.Trace($"Received message: messageId:{ brokeredMessage.MessageId} SequenceNumber:{brokeredMessage.SystemProperties.SequenceNumber}");


MessageId can be null. You want to wrap it with some sort of delimiters to avoid confusion in the output.
Eg: $"Received message: messageId:'{brokeredMessage.MessageId}' SequenceNumber....

SeanFeldman · 2017-09-12T22:00:07Z

src/Foundatio.AzureServiceBus/Messaging/AzureServiceBusMessageBus.cs

-            if (!String.IsNullOrEmpty(_options.TopicUserMetadata))
-                td.UserMetadata = _options.TopicUserMetadata;
+            //if (!String.IsNullOrEmpty(_options.TopicUserMetadata))
+            //    td.UserMetadata = _options.TopicUserMetadata;


If disabled because of issues in GH (like this one), ideally would be good to see the issue URL in a comment.

@shahbarkha Azure/azure-sdk-for-net#3635 Any idea if this is now supported?

@niemyjski Its still not supported!!

SeanFeldman · 2017-09-12T22:02:43Z

src/Foundatio.AzureServiceBus/Utility/AuthHelper.cs

+                    );
+
+                    // If the token isn't a valid string, throw an error.
+                    if (String.IsNullOrEmpty(result.AccessToken)) {


String.IsNullOrEmpty

should be

string.IsNullOrEmpty

@SeanFeldman Thanks Sean for reviewing this. I know I am asking for too much but can you please also take a glance at the functionality on how I am calling AbandonAsync, CompleteAsync using 2 different objects ( IQueueclient and IMessageReceiver )

await _queueClient.CompleteAsync(entry.Id).AnyContext();
await _messageReceiver.CompleteAsync(entry.Id).AnyContext();

As you noticed this library has 2 ways of pulling the items from the queue:

Push Strategy - Which IQueueClient provides by registering RegisterMessageHandler.

Pull Strategy - Which IMessageReceive provides using ReceiveAsync.

For some reason, in the new azure service bus dot net core library ReceiveAsync method has been removed from the IQueueClient and that leaves me to use the IMessageReceiver for fetching the item from the queue for the 2nd strategy.

Even though the endpoint for the queue is same if I try to call AbandonAsyc using the IMessageReceiver object for the queue item that has been dequeued using IQueueClient, I get "MessageLockLostException". Similary, If i try to call AbandonAsync using the IQueueClient object for the queue item that has been dequeued using IMessageReceiver, then also i get the same exception.
Because of this reason, I have to maintain some metadata in the QueueEntry to know if the queue item was dequeued using push or pull strategy so that I can call the AbandonAsync or CompleteAsync and even RenewLockAsync using appropriate objects ( either IQueueClient or IMessageReceiver ).

Do you know by any chance why there is a difference in behavior ? Shouldnt there be no exception if i try to call any of the above methods with any object.

The older version of the library used SBMP by default. The new one is using AMQP. With SBMP you could receive a message with one receiver and complete with another where with AMQP there's a receiver affinity. Meaning you have to complete/abandon/renew your message using the same receiver used to get the message to begin with.

As for the MessageAPI (registering message handler) and ReceiveAsync, it's all there, just slightly reshuffled. ReceiveAsync is now on MessageReceiver as it's considered a more "advanced" option (you control number of messages, write your own message pump, etc) and QueueClient is more of a "simple" scenario, intended to be used with the OOTB message pump registered with RegisterSessionHandler.

AMQP there's a receiver affinity. Is that for AMQP in general?

For the ASB .NET Standard client. Don't know if that's a general AMQP requirement or not.

…he ASB Queue

…mentation

shahbarkha · 2017-10-30T13:00:50Z

No Blake it’s not implemented yet in the new 2.0 version Thank you Sent from my iPhone - Barkha shah

…

On Oct 30, 2017, at 7:47 AM, Blake Niemyjski ***@***.***> wrote: @niemyjski commented on this pull request. In src/Foundatio.AzureServiceBus/Messaging/AzureServiceBusMessageBus.cs: > @@ -151,14 +204,14 @@ public class AzureServiceBusMessageBus : MessageBusBase<AzureServiceBusMessageBu if (_options.TopicEnableExpress.HasValue) td.EnableExpress = _options.TopicEnableExpress.Value; - if (!String.IsNullOrEmpty(_options.TopicUserMetadata)) - td.UserMetadata = _options.TopicUserMetadata; + //if (!String.IsNullOrEmpty(_options.TopicUserMetadata)) + // td.UserMetadata = _options.TopicUserMetadata; @shahbarkha Azure/azure-sdk-for-net#3635 Any idea if this is now supported? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

niemyjski

Looking good, There are some small things that need to be updated like the tests calling the wrong methods and I'm not sure if a queue instance is being returned. I know some of that is timeout related and we probably need to address that in the base test harness (making a timeout passed in, or we correct the min timespan passed in to us to be the min azure allowed timespan if 0 is passed). It looks like the foundatio lib needs to be updated to the latest version (6.0). I can help with that if you think it's good to be merged in. We could merge it to a branch and then rework logging and the configuration system.

niemyjski · 2017-10-30T12:48:08Z

samples/Foundatio.ReceiverConsole/App.config

@@ -0,0 +1,26 @@
+<?xml version="1.0" encoding="utf-8"?>


We should be able to remove the app.config completely in all projects

niemyjski · 2017-10-30T12:50:13Z

samples/Foundatio.ReceiverConsole/Foundatio.ReceiverConsole.csproj

+    <None Include="packages.config" />
+  </ItemGroup>
+  <ItemGroup>
+    <PackageReference Include="Foundatio.TestHarness" Version="5.1.1474" />


We should pull in Foundatio 6.0. We just need to add the following nuget config (https://github.com/FoundatioFx/Foundatio.Redis/blob/feature/logging/NuGet.config minus the loresoft on line 5)

I have removed the sample projects. Now I am referencing Foundatio 5.1.1562 which is the latest and the greatest. I dont see 6.0 in the Updates at all. Am I missing something

niemyjski · 2017-10-30T12:51:11Z

samples/Foundatio.ReceiverConsole/Program.cs

+
+
+            try {
+                var o = new Receiver();


We can now do static async Task MainAsync and remove the Getawaiter().GetResult(). As this was added in the latest .net core..

niemyjski · 2017-10-30T12:51:50Z

samples/Foundatio.ReceiverConsole/Properties/AssemblyInfo.cs

+using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
+
+// General Information about an assembly is controlled through the following


We should be able to drop all assembly info's as well or at least just provide a title and description as we do in the Foundatio Project (logging branch)

niemyjski · 2017-10-30T12:53:32Z

samples/Foundatio.ReceiverConsole/Receiver.cs

+    class Receiver {
+        private async Task TestTopic() {
+            IMessageBus messageBus =
+                new AzureServiceBusMessageBus(new AzureServiceBusMessageBusOptions() {


If we update to the latest foundatio 6, it brings in the logging changes and we've migrated everything to the new cross plat configuration system). We can accept the pr as is but if you wanted to tackle that it's pretty fun to play with and learn :)

niemyjski · 2017-10-30T12:58:49Z

src/Foundatio.AzureServiceBus/Messaging/AzureServiceBusMessageBus.cs

                if (_topicClient == null)
                    return;

-                _topicClient?.Close();
+                await _topicClient?.CloseAsync();


Need to ensure we have AnyContext() on all awaited async methods.

niemyjski · 2017-10-30T12:59:57Z

test/Foundatio.AzureServiceBus.Tests/Queues/AzureServiceBusQueueTests.cs

-                LoggerFactory = Log
-            });
-        }
+        //protected override IQueue<SimpleWorkItem> GetQueue(int retries = 0, TimeSpan? workItemTimeout = null, TimeSpan? retryDelay = null, int deadLetterMaxItems = 100, bool runQueueMaintenance = true) {


Are these tests even running since there is no Queue implementation being returned?

Yes they are all running with actual Queue parameters. They are commented out because they need to be supplied with the actual values mentioned in the configuration file. Once they are supplied all tests pass. If they are commented then the base queue implementation is returned. One needs to uncomment them for all the ASB specific tests to run.

niemyjski · 2017-10-30T13:00:23Z

test/Foundatio.AzureServiceBus.Tests/Queues/AzureServiceBusQueueTests.cs

        [Fact]
        public override Task WillWaitForItemAsync() {
-            return base.WillWaitForItemAsync();
+            return WillWaitItemAsync();


This is calling the wrong test method

I had to override the method because base class has some timeout values which does not work for ASB. Like you mentioned we need to supply min and max timeout value.

niemyjski · 2017-10-30T13:00:40Z

test/Foundatio.AzureServiceBus.Tests/Queues/AzureServiceBusQueueTests.cs

        public override Task WillNotWaitForItemAsync() {
-            return base.WillNotWaitForItemAsync();
+            return DontWaitForItemAsync();


This is calling the wrong test method

niemyjski · 2017-10-30T13:00:55Z

test/Foundatio.AzureServiceBus.Tests/Queues/AzureServiceBusQueueTests.cs

        public override Task CanRenewLockAsync() {
-            return base.CanRenewLockAsync();
+            return RenewLockAsync();


This is calling the wrong test method

-Added base project for benchmark. Still need to write code. -Removed the files from the Sample project.

- Using Microsoft.Extensions.Logging for logging - Fixed unit test for service message bus

niemyjski · 2017-11-08T14:07:52Z

src/Foundatio.AzureServiceBus/Messaging/AzureServiceBusMessageBus.cs

@@ -73,44 +71,34 @@ public class AzureServiceBusMessageBus : MessageBusBase<AzureServiceBusMessageBu

                _subscriptionClient.RegisterMessageHandler(OnMessageAsync, new MessageHandlerOptions(OnExceptionAsync) { MaxConcurrentCalls = maxConcurrentCalls, AutoComplete = autoComplete });
                sw.Stop();
-                _logger.Trace("Ensure topic subscription exists took {0}ms.", sw.ElapsedMilliseconds);
+                if (_logger.IsEnabled(LogLevel.Trace)) _logger.LogTrace("Ensure topic subscription exists took {ElapsedMilliseconds}", sw.ElapsedMilliseconds);


Looks good, I learned yesterday that if you do "{Duration:g}", sw.Elapsed you get really nice formatting for timespans :)

will make these changes. Thank you

niemyjski · 2017-11-08T14:09:26Z

src/Foundatio.AzureServiceBus/Queues/AzureServiceBusQueue.cs

            _queueClient.RegisterMessageHandler(async (msg, token) => {
-                _logger.Trace("WorkerLoop Signaled {_options.Name}", _options.Name);
+                if (_logger.IsEnabled(LogLevel.Trace)) _logger.LogTrace("WorkerLoop Signaled {.Name}", _options.Name);


We got an extra . in here by accident

niemyjski · 2017-11-08T14:11:00Z

test/Foundatio.AzureServiceBus.Tests/Queues/AzureServiceBusQueueTests.cs

@@ -165,7 +170,7 @@ public class AzureServiceBusQueueTests : QueueTestBase {
                var sw = Stopwatch.StartNew();
                var workItem = await queue.DequeueAsync(TimeSpan.FromMilliseconds(100));
                sw.Stop();
-                _logger.Trace("Time {0}", sw.Elapsed);
+                if (_logger.IsEnabled(LogLevel.Trace)) _logger.LogTrace("Time {Elapsed}", sw.Elapsed);


Should add the formatter for Elapsed to {Duration:g}

niemyjski

LGTM!!!!

niemyjski · 2017-11-08T14:12:29Z

Looks great!! Awesome job. Do you feel like we should merge it in or merge it into a branch? Guess we could merge it in and just bump the major again or put it in preview until the management stuff gets done

niemyjski · 2017-11-08T14:13:54Z

@SeanFeldman would you mind reviewing this and leaving your comments to my last question :) Thanks in advance

niemyjski · 2017-11-10T13:32:20Z

Let me know when you want me to merge this in :)

shahbarkha · 2017-11-10T18:26:15Z

@niemyjski I liked your idea of putting this stuff on the preview untill the management stuff is released. Also I want to do the benchmark thing going on before we merge it in the master.

niemyjski · 2017-11-14T11:28:28Z

Thanks for the PR! I've merged this into the .net standard branch. We'll push a preview build out shortly. We might have to wait until the logging release goes out as that's going to be a stable build and then this one so it's a higher nightly preview build or bump the major on this

SeanFeldman · 2017-11-14T16:16:32Z

@SeanFeldman would you mind reviewing this and leaving your comments to my last question :) Thanks in advance

Sorry @niemyjski, was away on vacation. Looks like a bit late to review 😃

shahbarkha added 3 commits August 5, 2017 16:57

first cut for microsoft.azure.servicebus with new management

30248d8

some conditional checks

2c2da32

added sample app for topic and queue and revised the new azure librar…

553f70e

…y version to 1.0.0 from 0.0.7 preview

removed obsolete constructors

3d7599a

niemyjski requested a review from jamie94bc August 15, 2017 12:21

shahbarkha added 8 commits August 17, 2017 12:13

added tokencredentials in the library, changed sample project to netc…

742f101

…oreapp, and added few new options to the azuremessagebusoptions.

Missed out on using the subscription name as a member variable. If th…

aba238b

…e user does not provide any subscription name then by default guid is generated and topic is used with fanout mode

some options of SBSubscritpiton will be added in the later release of…

953d01d

… library. putting the comment for the github issue.

Fixed few issues found during testing for queues and topic. Enhanced …

6b9d889

…sample examples.

corrected the logic to delete the queue

e0c5c0d

upgraded assemblies to use dotnetcore2.0

3c452d0

fixed bug found during unit test of CanHandleErrorInWorkerAsync method.

adc8e6a

made some changes in the queue implementation to handle calling aband…

6e2725c

…onasync, completeasync on different objects bases on the push or pull strategy.

niemyjski mentioned this pull request Sep 6, 2017

SubscriptionDescription vs SBSubscription - missing parameters Azure/azure-service-bus-dotnet#255

Closed

if Timespan.zero is passed in ReceiveAsync then Azure library throws …

adf4aa6

…exception. We instead call the overloaded method with no timeout in that case.

shahbarkha mentioned this pull request Sep 12, 2017

Management Operations Azure/azure-service-bus-dotnet#65

Closed

SeanFeldman reviewed Sep 12, 2017

View reviewed changes

shahbarkha added 3 commits October 24, 2017 12:55

Updated ASB library with 2.0, made sure all the unit tests pass for t…

3d89644

…he ASB Queue

nugetpackage update

799bd97

Correctly handled AutoComplete feature in the StartWorkingAsync imple…

374487a

…mentation

niemyjski requested changes Oct 30, 2017

View reviewed changes

shahbarkha added 3 commits October 31, 2017 12:18

- Updated Foundation to 5.1.1562

d26f0e4

-Added base project for benchmark. Still need to write code. -Removed the files from the Sample project.

fixed Dispose method for both the message bus and message queue

b5d31e1

-updated foundatio to 6.0.1557-pre

2d37b3a

- Using Microsoft.Extensions.Logging for logging - Fixed unit test for service message bus

niemyjski reviewed Nov 8, 2017

View reviewed changes

niemyjski approved these changes Nov 8, 2017

View reviewed changes

shahbarkha added 2 commits November 8, 2017 12:08

fixed some logging formatters as suggested by Blake

003238f

removed package source from the appveyor.yml

4321d14

niemyjski changed the base branch from master to feature/netstandard November 14, 2017 11:26

niemyjski merged commit fa28eab into FoundatioFx:feature/netstandard Nov 14, 2017

		@@ -0,0 +1,83 @@
		<?xml version="1.0" encoding="utf-8"?>
		<packages>

updated service bus with new microsoft.azure.servicebus dotnet library #1

updated service bus with new microsoft.azure.servicebus dotnet library #1

Conversation

shahbarkha commented Aug 14, 2017 • edited Loading

CLAassistant commented Aug 14, 2017 • edited Loading

SeanFeldman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahbarkha commented Oct 30, 2017 via email

niemyjski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemyjski left a comment

Choose a reason for hiding this comment

niemyjski commented Nov 8, 2017

niemyjski commented Nov 8, 2017

niemyjski commented Nov 10, 2017

shahbarkha commented Nov 10, 2017

niemyjski commented Nov 14, 2017 • edited Loading

SeanFeldman commented Nov 14, 2017

shahbarkha commented Aug 14, 2017 •

edited

Loading

CLAassistant commented Aug 14, 2017 •

edited

Loading

niemyjski commented Nov 14, 2017 •

edited

Loading