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

Sender updates #11580

Merged
merged 4 commits into from
Apr 27, 2020
Merged

Sender updates #11580

merged 4 commits into from
Apr 27, 2020

Conversation

JoshLove-msft
Copy link
Member

@JoshLove-msft JoshLove-msft commented Apr 24, 2020

  • Add ServiceBusSenderOptions
  • Rename SendBatchAsync -> SendAsync

@JoshLove-msft
Copy link
Member Author

/azp run net - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Comment on lines 194 to 195
ValidateEntityName(options?.ViaQueueOrTopicName);
ValidateEntityName(queueOrTopicName);
Copy link
Member

Choose a reason for hiding this comment

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

I think we shouldn't call ValidateEntityName method for queueOrTopicName and ViaQueueOrTopicName both. Think about if user passes EntityPath with connection string, then one of the above method will fail because we check in ValidateEntityName method that if the entity path specified in both(Connection.EntityPath and entityName) with different value then it will throw an exception.

if (!string.IsNullOrEmpty(Connection.EntityPath) && !string.Equals(entityName, Connection.EntityPath, StringComparison.InvariantCultureIgnoreCase))
{
      throw new ArgumentException(Resources.OnlyOneEntityNameMayBeSpecified);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, if they pass an entity name in the connection string that means they won't be able to use SendVia, since they are restricted to that one entity. So in this case we would want to throw an exception.

Copy link
Member

Choose a reason for hiding this comment

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

Did you forget to update this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated!

Comment on lines +12 to +16
/// A set of <see cref="ServiceBusMessage" /> with size constraints known up-front,
/// intended to be sent to the Queue/Topic as a single batch.
/// A <see cref="ServiceBusMessageBatch"/> can be created using
/// <see cref="ServiceBusSender.CreateBatchAsync(System.Threading.CancellationToken)"/>.
/// Messages can be added to the batch using the <see cref="TryAdd"/> method on the batch.
Copy link
Member

Choose a reason for hiding this comment

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

Nice!!

Copy link
Member

Choose a reason for hiding this comment

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

Can we add this Messages can be added to the batch using the <see cref="TryAdd"/> method on the batch for CreateBatchAsync docs as well?

Comment on lines 87 to 88
/// <param name="options">The set of <see cref="ServiceBusClientOptions"/> to use for configuring
/// this <see cref="ServiceBusClient"/>.</param>
Copy link
Member

Choose a reason for hiding this comment

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

nit: ServiceBusClientOptions -> ServiceBusSenderOptions and ServiceBusClient -> ServiceBusSender

@@ -216,7 +216,7 @@ messageBatch.TryAdd(new ServiceBusMessage(Encoding.UTF8.GetBytes("First")));
messageBatch.TryAdd(new ServiceBusMessage(Encoding.UTF8.GetBytes("Second")));

// send the message batch
await sender.SendBatchAsync(messageBatch);
await sender.SendAsync(messageBatch);
Copy link
Member

@ShivangiReja ShivangiReja Apr 24, 2020

Choose a reason for hiding this comment

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

Can we update SendBatchAsync -> SendAsync in the Sending messages section and in the description of Sending and receiving batch of messages?

@JoshLove-msft JoshLove-msft merged commit 7cbe9dc into Azure:master Apr 27, 2020
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.

2 participants