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

[ServiceBus] Update migration guide #15915

Merged

Conversation

yunhaoling
Copy link
Contributor

@yunhaoling yunhaoling commented Dec 24, 2020

addressing: #15106 and #14202

| `ServiceBusClient.from_connection_string()` | `ServiceBusClient.from_connection_string()` | [using credential](https://github.com/Azure/azure-sdk-for-python/tree/master/sdk/servicebus/azure-servicebus/samples/sync_samples/authenticate_client_connstr.py ) |
| `QueueClient.from_connection_string()` | `ServiceBusClient.from_connection_string().get_queue_<sender/receiver>()` | [client initialization](https://github.com/Azure/azure-sdk-for-python/tree/master/sdk/servicebus/azure-servicebus/samples/sync_samples/send_queue.py ) |
| `QueueClient.from_connection_string(idle_timeout=None)` | `ServiceBusClient.from_connection_string.get_queue_receiver(max_wait_time=None)` | [providing a timeout](https://github.com/Azure/azure-sdk-for-python/tree/master/sdk/servicebus/azure-servicebus/samples/sync_samples/session_pool_receive.py) |
In V0.50:
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason the table format is changed? Also, I think there's a great value in providing the relevant sample in place

Copy link
Contributor Author

@yunhaoling yunhaoling Jan 4, 2021

Choose a reason for hiding this comment

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

I totally agree with you that table has the advantage for comparison.

but what I found is that table might be good for comparison of short/brief code snippets, but too crowded for large content.

for example this one:
image

I think the readability decreases as we want to show more contents in a table. one approach might be we further compress the code snippets, but flattening it out also help users who just want to copy and past the code.


actually, I think Message properties migration guide segment is a perfect case to apply table.
we could discuss further which sections are suitable for table, and see how others are doing.

Co-authored-by: Rakshith Bhyravabhotla <rakshith.bhyravabhotla@gmail.com>
Co-authored-by: swathipil <76007337+swathipil@users.noreply.github.com>
sdk/servicebus/azure-servicebus/migration_guide.md Outdated Show resolved Hide resolved
sdk/servicebus/azure-servicebus/migration_guide.md Outdated Show resolved Hide resolved
sdk/servicebus/azure-servicebus/migration_guide.md Outdated Show resolved Hide resolved
subscription_dead_letter_receiver = subscription_client.get_deadletter_receiver()
```

In V7:
```Python
with ServiceBusClient.from_connection_string(connstr) as client:
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to use the with construct in the v7 code snippet but not in v0.50?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's because v5 ServiceBusClient doesn't support the with context manager.
but I tend to keep the usage of with for v7 for consistency with other snippets.

sdk/servicebus/azure-servicebus/migration_guide.md Outdated Show resolved Hide resolved
# receiver.abandon_message(received_message)
# receiver.defer_message(received_message)
# receiver.dead_letter_message(received_message)
```

### Renewing lock on the received message
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we combine the auto lock renewal section in here? Since both topics are about lock renewals it would make sense to have a single section called Lock renewal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good suggestion, I tried to combine them into one, let me know how it looks to you.

receiver.renew_message_lock(received_message)
```

### Scheduling messages and cancelling scheduled messages
Copy link
Contributor

Choose a reason for hiding this comment

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

There is not much in this section other than renames. In which case I would either recommend not calling them out at all or having a separate section called "Renames" (or something nicer :) ) and put these there. Having a dedicated section is helpful if the method has moved or has changed in what it accepts as input or changes what it returns as output.

Other sections that fall under the same category are the ones for utc time and for receiving messages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a change in the schedule/cancel schedule api that in v0.50 the method takes argument list while in v7 it takes list so I'd like to keep it.

I have removed the utc time section; as for the receiving messages sections since it's a champion scenario I tend to keep it as it is.

sdk/servicebus/azure-servicebus/migration_guide.md Outdated Show resolved Hide resolved
sdk/servicebus/azure-servicebus/migration_guide.md Outdated Show resolved Hide resolved
and [Async API](https://azuresdkdocs.blob.core.windows.net/$web/python/azure-servicebus/latest/azure.servicebus.aio.html#azure.servicebus.aio.ServiceBusSender)
* `ServiceBusReceiver` for receiving messages. [Sync API](https://azuresdkdocs.blob.core.windows.net/$web/python/azure-servicebus/latest/azure.servicebus.html#azure.servicebus.ServiceBusReceiver)
and [Async API](https://azuresdkdocs.blob.core.windows.net/$web/python/azure-servicebus/latest/azure.servicebus.aio.html#azure.servicebus.aio.ServiceBusReceiver)
In V0.50:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking this PR, but thoughts for a future PR

We want to show 3 different things here

  • That constructing the client using connection string has not changed
  • Constructing client using SAS key and name is an upcoming feature
  • Using AAD is a brand new feature

Consider treating these separately. For example:

Constructing the client using the connection string is same in both versions.
servicebus_client = ServiceBusClient.from_connection_string(conn_str)

Additionally, you can now use Azure Active Directory for authentication via the azure-identity library
show code for AAD here

The option to construct the client using SAS key name and value is not available in the new version, but will be added in the near future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I love the way you organize the concepts, I have copied your words and tweaked a bit.

sdk/servicebus/azure-servicebus/migration_guide.md Outdated Show resolved Hide resolved
@yunhaoling yunhaoling merged commit e5b7b5c into Azure:master Jan 6, 2021
rakshith91 added a commit to rakshith91/azure-sdk-for-python that referenced this pull request Jan 8, 2021
* update migration guide

* Apply suggestions from code review

Co-authored-by: Rakshith Bhyravabhotla <rakshith.bhyravabhotla@gmail.com>
Co-authored-by: swathipil <76007337+swathipil@users.noreply.github.com>

* addressing comments

* tweak table of contents

* update according to comments

Co-authored-by: Rakshith Bhyravabhotla <rakshith.bhyravabhotla@gmail.com>
Co-authored-by: swathipil <76007337+swathipil@users.noreply.github.com>
@yunhaoling yunhaoling deleted the yuling/sb/migration-guide-update branch January 13, 2021 01:17
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-python that referenced this pull request Nov 9, 2021
[Hub Generated] Review request for Microsoft.PowerBIdedicated to add version stable/2017-10-01 (Azure#15915)

* Update missing properties in PowerBIDedicated objects 2017-10-01

* Move PowerBIDedicated properties to new definitions

* Realign error response to recommended schema. 202 response not necessary in older api-version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants