[azservicebus] Final API changes for beta.3 #16110

richardpark-msft · 2021-11-07T20:30:45Z

Lots of renames and changes based on the API reviews:

Admin API:

All Response structs have been broken down into Response+Result
QueueName, TopicName, SubscriptionName are no longer present in the Properties structs as that doesn't reflect the API itself.
Exists functions have been removed
Missing fields have been added (UserMetadata, etc..)

Sender and Receiver API:

SendMessages removed in favor of making SendMessageBatch more intuitive (by simplfying the .AddMessage API)
Several renames suggested in review
ReceiveMessages has been tuned to match the .NET limits (which has worked well in practice). This is partly addressing Make maxWaitTimeAfterFirstMessage configurable for Servicebus Receive function #15963, as our default limit was far higher than needed.

Missing:

AuthorizationRules, SubscriptionRules (coming in a future PR but requires a bit more thought)

richardpark-msft · 2021-11-07T20:31:53Z

/azp run go - azservicebus

azure-pipelines · 2021-11-07T20:32:07Z

Azure Pipelines successfully started running 1 pipeline(s).

…e drain any messages that have arrived as we drain a link (this was a small, if possibly non-existent window, but it's good bulletproofing) - Adding in PropertiesToModify options for DeferMessage, and AbandonMessage, allowing users to have properties modified service-side when the message is moved/returned. - Adding in another short-lived stress test - send 10000ish messages, and receive them all. Need some more work, but we're just creating some skeletons. - Adding in missing properties for entities that are present like UserMetadata, AutoDeleteOnIdle, etc... Still missing: AuthorizationRules, and TopicRules. - Many many renames based on API feedback. - Eliminating $skip parameter, treating $top as "max page size" - Simple consistency renames when you specify the name for an entity (ie, topic => topicName, etc..) - Responses from Admin clients now conform to standard - Responses embed a Result which embeds the natural return value (ie: QueueProperties). - Lots of future proofing *options parameters, where we think we might eventually have some options. - ReceivedMessage.Body is now a function, rather than a field. This protects against a situation where, if a message was crafted directly using an AMQP stack, it could encode the Body into a format we couldn't represent. This will not return an error. When we add in low level AMQP support these messages will be readable through there.

… we get AMQP support in.

…he customer knows they can call it multiple times, and how it signals when there's a fatal error or just that the batch is full.

…te the list of upcoming features.

- Adding panicky stub for GetNamespaceProperties (about to implement it) - Adding future expansions options to Delete<Entity> functions.

…both azservicebus and admin

richardpark-msft · 2021-11-08T02:05:32Z

/azp run go - azservicebus

azure-pipelines · 2021-11-08T02:05:46Z

Azure Pipelines successfully started running 1 pipeline(s).

richardpark-msft · 2021-11-08T03:23:53Z

/azp run go - azservicebus

azure-pipelines · 2021-11-08T03:24:08Z

Azure Pipelines successfully started running 1 pipeline(s).

richardpark-msft · 2021-11-08T22:15:06Z

/azp run go - azservicebus

azure-pipelines · 2021-11-08T22:15:21Z

Azure Pipelines successfully started running 1 pipeline(s).

richardpark-msft · 2021-11-08T23:43:40Z

/azp run go - azservicebus

azure-pipelines · 2021-11-08T23:43:54Z

Azure Pipelines successfully started running 1 pipeline(s).

sdk/messaging/azservicebus/internal/mgmt.go

…a pointer. - adding in some unit tests for the pagers.

sdk/messaging/azservicebus/admin/admin_client.go

sdk/messaging/azservicebus/admin/main_test.go

sdk/messaging/azservicebus/client.go

sdk/messaging/azservicebus/message.go

sdk/messaging/azservicebus/messageBatch.go

richardpark-msft · 2021-11-09T18:24:23Z

/azp run go - azservicebus

azure-pipelines · 2021-11-09T18:24:39Z

Azure Pipelines successfully started running 1 pipeline(s).

richardpark-msft · 2021-11-09T21:34:39Z

/azp run go - azservicebus

azure-pipelines · 2021-11-09T21:34:56Z

Azure Pipelines successfully started running 1 pipeline(s).

sdk/messaging/azservicebus/admin/admin_client_subscription.go

sdk/messaging/azservicebus/receiver.go

sdk/messaging/azservicebus/sender.go

richardpark-msft · 2021-11-09T22:25:54Z

/azp run go - azservicebus

azure-pipelines · 2021-11-09T22:26:10Z

Azure Pipelines successfully started running 1 pipeline(s).

… was unclear/confusing.

richardpark-msft · 2021-11-10T00:11:47Z

/azp run go - azservicebus

azure-pipelines · 2021-11-10T00:12:04Z

Azure Pipelines successfully started running 1 pipeline(s).

richardpark-msft · 2021-11-10T00:28:03Z

/azp run go - azservicebus

azure-pipelines · 2021-11-10T00:28:19Z

Azure Pipelines successfully started running 1 pipeline(s).

… page is < max page size that's the last page, don't do further requests after that. Also, unified all the pagers under one function instead of separate little pager functions.

richardpark-msft · 2021-11-10T02:03:15Z

/azp run go - azservicebus

azure-pipelines · 2021-11-10T02:03:31Z

Azure Pipelines successfully started running 1 pipeline(s).

sdk/messaging/azservicebus/admin/admin_client_queue.go

jhendrixMSFT · 2021-11-09T01:38:34Z

sdk/messaging/azservicebus/admin/admin_client_subscription.go

-func (ac *AdminClient) ListSubscriptions(topicName string, options *ListSubscriptionsOptions) SubscriptionPropertiesPager {
-	var pageSize int
-	var skip int
+func (ac *Client) ListSubscriptions(topicName string, options *ListSubscriptionsOptions) SubscriptionPropertiesPager {


Don't return an interface here.

jhendrixMSFT · 2021-11-09T01:43:07Z

sdk/messaging/azservicebus/message.go

+}
+
+// Body returns the body for this received message.
+// If the body not compatible with ReceivedMessage this function will return an error.


When would be the body not be compatible (what does that even mean)? Is the error in this case useful or can we just return a nil slice?

I'll clarify it when I actually add in AMQP support which I'm doing now. I didn't want to add too much info here until then.

What I will eventually write is:

The body (in an AMQP message) can be in three separate spots - in the Value section, the Data section or in a Sequence section. Each section has a possible value that will not cleanly encode to a single []byte array.

Data -> can contain multiple byte arrays
Value -> can contain AMQP primitives, etc..
Sequence -> can be a sequence that's not bytes

jhendrixMSFT · 2021-11-09T01:44:10Z

sdk/messaging/azservicebus/message.go

+
+	ContentType   string
+	CorrelationID string
+	// Body corresponds to the first []byte array in the Data section of an AMQP message.


Can this comment be removed?

jhendrixMSFT · 2021-11-09T01:52:21Z

sdk/messaging/azservicebus/sender.go

@@ -53,18 +45,18 @@ func (s *Sender) NewMessageBatch(ctx context.Context, options *MessageBatchOptio
 		return nil, err
 	}

-	maxBytes := int(sender.MaxMessageSize())
+	maxBytes := int32(sender.MaxMessageSize())


Seems weird we have to down-cast here. Makes me think we should change this in go-amqp to a uint32, although you'd still have to cast here.

Let me check what the actual native type is. It might be that I need to go the other way, even if, in reality, the domain doesn't support a crazy large max message size.

So this is an AMQP field (straight from the attach frame) and it's typed to be a ulong, not a long.

http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-transport-v1.0-os.html#:~:text=See%20subsection%202.6.7.-,max-message-size,-the%20maximum%20message

http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-types-v1.0-os.html#type-ulong

So we should probably follow suit there, but int32 is definitely wrong.

sdk/messaging/azservicebus/sender.go

Lots of renames and changes based on the API reviews: Admin API: - All `Response` structs have been broken down into `Response+Result` - QueueName, TopicName, SubscriptionName are no longer present in the Properties structs as that doesn't reflect the API itself. - <Entity>Exists functions have been removed - Missing fields have been added (UserMetadata, etc..) Sender and Receiver API: - SendMessages removed in favor of making SendMessageBatch more intuitive (by simplfying the .AddMessage API) - Several renames suggested in review - ReceiveMessages has been tuned to match the .NET limits (which has worked well in practice). This is partly addressing Azure#15963, as our default limit was far higher than needed. Coming in near term PRs: - AuthorizationRules, SubscriptionRules (coming in a future PR but requires a bit more thought) -

richardpark-msft requested a review from jhendrixMSFT as a code owner November 7, 2021 20:30

richardpark-msft marked this pull request as draft November 7, 2021 20:32

richardpark-msft added 2 commits November 7, 2021 21:32

Removing function that's not quite ready for prime time.

f226bb7

richardpark-msft force-pushed the jeffr-api-changes branch from 4f8a00e to f226bb7 Compare November 7, 2021 21:41

richardpark-msft added 9 commits November 7, 2021 21:46

Cleaning up the message a little. It'll become more prescriptive once…

0d71b5a

… we get AMQP support in.

Updating example to be simpler - really the important stuff is that t…

ab0edde

…he customer knows they can call it multiple times, and how it signals when there's a fatal error or just that the batch is full.

Updating migration guide to be accurate against the API surface, upda…

8c35ba0

…te the list of upcoming features.

moving admin client into a sub-package

c6fce94

- AdminClient moved into it's own sub-package.

81cbf4b

- Adding panicky stub for GetNamespaceProperties (about to implement it) - Adding future expansions options to Delete<Entity> functions.

Move stress into the internal package

2b23de8

Fixing some linting failures, updating the package level comment for …

9edc8ee

…both azservicebus and admin

Adding in getting namespace properties to the admin.Client

2ca93bc

Used the wrong format for the forwarding entity

edc56f1

Update copyright header

b4e4392

This is going to be a beta release, so we'll bump the minor revision.

aaa3517

richardpark-msft marked this pull request as ready for review November 8, 2021 19:36

Updating tests and options name

c5d0d01

jhendrixMSFT reviewed Nov 9, 2021

View reviewed changes

sdk/messaging/azservicebus/internal/mgmt.go Outdated Show resolved Hide resolved

- propertiesToModify is already a nullable value, no need to make it …

7fe92cc

…a pointer. - adding in some unit tests for the pagers.

chlowell reviewed Nov 9, 2021

View reviewed changes

Richard Park added 4 commits November 9, 2021 10:01

Fixing note to mention 'azservicebus.Client'

8bc310c

Changing 'an Client' to 'a Client'

f3bc0f9

NamespaceProperties.RawResult => NamespaceProperties.RawResponse

02a1e90

Remove unneeded .Sequence comment

5e6f237

seankane-msft approved these changes Nov 9, 2021

View reviewed changes

chlowell approved these changes Nov 9, 2021

View reviewed changes

sdk/messaging/azservicebus/admin/admin_client_subscription.go Show resolved Hide resolved

sdk/messaging/azservicebus/receiver.go Show resolved Hide resolved

sdk/messaging/azservicebus/sender.go Outdated Show resolved Hide resolved

Alter our counting to always use maxPageSize as the increment.

798aecf

richardpark-msft force-pushed the jeffr-api-changes branch from 8e7902a to 798aecf Compare November 10, 2021 00:03

Richard Park added 2 commits November 9, 2021 16:08

Fixing tests

1a33990

Adding some comments and explanation for a bit that Charles mentioned…

af06fed

… was unclear/confusing.

Remove dead comment about AMQPAnnotatedMessage

5ab8104

Richard Park added 2 commits November 9, 2021 16:31

Updating changelog with a note that we've moved the admin client

a981633

Updating all pagers to match the logic in the .NET client, where if a…

52e7a04

… page is < max page size that's the last page, don't do further requests after that. Also, unified all the pagers under one function instead of separate little pager functions.

richardpark-msft merged commit ae68cfe into Azure:main Nov 10, 2021

richardpark-msft deleted the jeffr-api-changes branch November 10, 2021 18:11

jhendrixMSFT reviewed Nov 10, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[azservicebus] Final API changes for beta.3 #16110

[azservicebus] Final API changes for beta.3 #16110

richardpark-msft commented Nov 7, 2021

richardpark-msft commented Nov 7, 2021

azure-pipelines bot commented Nov 7, 2021

richardpark-msft commented Nov 8, 2021

azure-pipelines bot commented Nov 8, 2021

richardpark-msft commented Nov 8, 2021

azure-pipelines bot commented Nov 8, 2021

richardpark-msft commented Nov 8, 2021

azure-pipelines bot commented Nov 8, 2021

richardpark-msft commented Nov 8, 2021

azure-pipelines bot commented Nov 8, 2021

richardpark-msft commented Nov 9, 2021

azure-pipelines bot commented Nov 9, 2021

richardpark-msft commented Nov 9, 2021

azure-pipelines bot commented Nov 9, 2021

richardpark-msft commented Nov 9, 2021

azure-pipelines bot commented Nov 9, 2021

richardpark-msft commented Nov 10, 2021

azure-pipelines bot commented Nov 10, 2021

richardpark-msft commented Nov 10, 2021

azure-pipelines bot commented Nov 10, 2021

richardpark-msft commented Nov 10, 2021

azure-pipelines bot commented Nov 10, 2021

jhendrixMSFT Nov 9, 2021

jhendrixMSFT Nov 9, 2021

richardpark-msft Nov 10, 2021

jhendrixMSFT Nov 9, 2021

jhendrixMSFT Nov 9, 2021

richardpark-msft Nov 10, 2021

richardpark-msft Nov 10, 2021

[azservicebus] Final API changes for beta.3 #16110

[azservicebus] Final API changes for beta.3 #16110

Conversation

richardpark-msft commented Nov 7, 2021

richardpark-msft commented Nov 7, 2021

azure-pipelines bot commented Nov 7, 2021

richardpark-msft commented Nov 8, 2021

azure-pipelines bot commented Nov 8, 2021

richardpark-msft commented Nov 8, 2021

azure-pipelines bot commented Nov 8, 2021

richardpark-msft commented Nov 8, 2021

azure-pipelines bot commented Nov 8, 2021

richardpark-msft commented Nov 8, 2021

azure-pipelines bot commented Nov 8, 2021

richardpark-msft commented Nov 9, 2021

azure-pipelines bot commented Nov 9, 2021

richardpark-msft commented Nov 9, 2021

azure-pipelines bot commented Nov 9, 2021

richardpark-msft commented Nov 9, 2021

azure-pipelines bot commented Nov 9, 2021

richardpark-msft commented Nov 10, 2021

azure-pipelines bot commented Nov 10, 2021

richardpark-msft commented Nov 10, 2021

azure-pipelines bot commented Nov 10, 2021

richardpark-msft commented Nov 10, 2021

azure-pipelines bot commented Nov 10, 2021

jhendrixMSFT Nov 9, 2021

Choose a reason for hiding this comment

jhendrixMSFT Nov 9, 2021

Choose a reason for hiding this comment

richardpark-msft Nov 10, 2021

Choose a reason for hiding this comment

jhendrixMSFT Nov 9, 2021

Choose a reason for hiding this comment

jhendrixMSFT Nov 9, 2021

Choose a reason for hiding this comment

richardpark-msft Nov 10, 2021

Choose a reason for hiding this comment

richardpark-msft Nov 10, 2021

Choose a reason for hiding this comment