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] set timeout on link properties #30832

Merged
merged 50 commits into from
Feb 2, 2024

Conversation

l0lawrence
Copy link
Member

@l0lawrence l0lawrence commented Jun 21, 2023

fixes #26056 and is related to questions raised in #29878 and #30907

fixes #11505

This PR adds in a timeout property on the receiver link, this timeout property only comes into effect on Session-Enabled Queues/Topics when NEXT_AVAILABLE_SESSION has been set. If you set max_wait_time (or whichever variable if we want to create a new one) when receiving it will be passed in through the receiverLink and the server will timeout and return after that time has been reached. This allows for customers to wait for less than the standard 65 seconds that the server default sets.

Update email threads for customers when merged

What is being Changed:

  • ServiceBusReceiver.receive_messages() max_wait_time remains unchanged in behavior
  • ServiceBusReceiver max_wait_time now will impact connecting to NEXT_AVAILABLE_SESSION time

@azure-sdk
Copy link
Collaborator

azure-sdk commented Jun 21, 2023

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-servicebus

@l0lawrence l0lawrence changed the title [ServiecBus] attempt to set timeout on link properties [ServiecBus] set timeout on link properties Jun 21, 2023
@l0lawrence
Copy link
Member Author

/azp run python - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@l0lawrence
Copy link
Member Author

/azp run python - servicebus - tests

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 30832 in repo Azure/azure-sdk-for-python

@l0lawrence l0lawrence changed the title [ServiecBus] set timeout on link properties [ServiceBus] set timeout on link properties Jul 7, 2023
@l0lawrence
Copy link
Member Author

/azp run python - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@swathipil swathipil left a comment

Choose a reason for hiding this comment

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

left a few comments, thanks @l0lawrence!

@l0lawrence l0lawrence marked this pull request as ready for review July 7, 2023 20:22
@l0lawrence
Copy link
Member Author

/azp run python - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@l0lawrence
Copy link
Member Author

/azp run python - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ValMobBIllich
Copy link

Heyyy guys. We ran into the same problem as the two issues that caused this PR.
So far we managed to pry open the sdk and just manually inserted a timeout into _open() (which I think might be like an ugly version of what you did?) but of course wed be more happy to just use a newer offical version of the sdk instead.

Is there any way that I can help close this PR? I havent contributed anything to the azure sdk but would love to learn how to!

@l0lawrence
Copy link
Member Author

Heyyy guys. We ran into the same problem as the two issues that caused this PR. So far we managed to pry open the sdk and just manually inserted a timeout into _open() (which I think might be like an ugly version of what you did?) but of course wed be more happy to just use a newer offical version of the sdk instead.

Is there any way that I can help close this PR? I havent contributed anything to the azure sdk but would love to learn how to!

Hi @ValMobBIllich , thanks for chiming in! Here is a ⁠doc that describes how to be an active contributor to the azure-sdk-for-python-repo. For this PR in particular there isn't anything else needed code-wise, but once we get to a final design point -- if you would be willing to help test this in your current environment that would be extremely helpful!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
nit
nit
nit
doc
@l0lawrence
Copy link
Member Author

/azp run python - servicebus - tests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@swathipil swathipil left a comment

Choose a reason for hiding this comment

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

left a few comments but overall this looks v good!

l0lawrence and others added 3 commits January 31, 2024 10:15
Co-authored-by: swathipil <76007337+swathipil@users.noreply.github.com>
@l0lawrence l0lawrence merged commit 81b6a38 into Azure:main Feb 2, 2024
15 checks passed
sofiar-msft pushed a commit to sofiar-msft/azure-sdk-for-python that referenced this pull request Feb 16, 2024
* attempt to set timeout on link properties

* jitter math

* update receiver logic

* update encode

* move logic to pyamqp_transport

* update docstring

* add tests

* update if statement

* update if statements

* whitespace

* trailing whitespace

* move logic into pyamqp_transport

* pylint

* update kwargs

* Update sdk/servicebus/azure-servicebus/azure/servicebus/_servicebus_receiver.py

Co-authored-by: Kashif Khan <361477+kashifkhan@users.noreply.github.com>

* update async docstring too

* update docstrings

* add back in jitter logic to align with other lang

* update client wording

* update other receiver docstring

* remove spacing

* pylint

* add comment

* update client

* fix pylint

* update max_wait_time docstring

* align iterator and receive_messages() behavior

* docstring updates

* asyn align iterator and receive_messages()

* add deprecation

* pylint fixes

* add timeout kwarg

* update docstring

* client docstring

* add to OperationTimeoutError logic

* remove timeout from mock

* nit

* nit

* revert doc

* update error msg

* nit

* base handler async handling

* doc

* pylint

* pylint

* conn str doc update

* remove ==True

Co-authored-by: swathipil <76007337+swathipil@users.noreply.github.com>

* add >65 to test

* add warning

* move note to docstring

---------

Co-authored-by: Kashif Khan <361477+kashifkhan@users.noreply.github.com>
Co-authored-by: swathipil <76007337+swathipil@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants