Add ability to set max calls across all sessions#23282
Closed
JoshLove-msft wants to merge 14 commits intoAzure:mainfrom
Closed
Add ability to set max calls across all sessions#23282JoshLove-msft wants to merge 14 commits intoAzure:mainfrom
JoshLove-msft wants to merge 14 commits intoAzure:mainfrom
Conversation
ghost
added
the
Member
Author
|
Actually, hold off on reviewing please - need to make a few more updates. |
Member
Author
|
/azp run net - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
JoshLove-msft
requested review from
KrzysztofCwalina and
tg-msft
as code owners
Member
Author
|
/azp run net - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
JoshLove-msft
commented
Aug 12, 2021
sdk/servicebus/Azure.Messaging.ServiceBus/api/Azure.Messaging.ServiceBus.netstandard2.0.cs
Show resolved
Hide resolved
Member
Author
|
/azp run net - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
Author
|
/azp run net - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
Author
|
/azp run net - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
mathewc
reviewed
Aug 13, 2021
sdk/servicebus/Azure.Messaging.ServiceBus/src/Processor/ServiceBusProcessor.cs
Outdated
Show resolved
Hide resolved
mathewc
reviewed
Aug 13, 2021
sdk/servicebus/Azure.Messaging.ServiceBus/src/Processor/ServiceBusProcessor.cs
Show resolved
Hide resolved
Member
Author
|
/azp run net - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
Author
|
/azp run net - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
Author
|
/azp run net - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
Author
|
/azp run net - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
Author
|
/azp run net - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
Author
|
It looks like a lot of the flakiness around the session concurrency tests is related to how we were implementing the "fake" cancellation support when accepting sessions. Now that the AMQP lib has been updated to have real cancellation token support, I've updated the OpenLink flows to use it. I've created an issue to track the rest of the updates that are needed - #23333 |
Member
Author
|
/azp run net - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
Author
|
/azp run net - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jsquire
approved these changes
Aug 16, 2021
| Source = new Source { Address = endpoint.AbsolutePath, FilterSet = filters }, | ||
| Target = new Target { Address = Guid.NewGuid().ToString() } | ||
| Target = new Target { Address = Guid.NewGuid().ToString() }, | ||
| OperationTimeout = _operationTimeout |
| /// accept session requests. | ||
| /// </summary> | ||
| private SemaphoreSlim MaxConcurrentAcceptSessionsSemaphore { get; } | ||
| private readonly SemaphoreSlim _maxConcurrentAcceptSessionsSemaphore = new(0, int.MaxValue); |
Member
There was a problem hiding this comment.
I'm guessing we don't want to recreate this when concurrency is changed?
Member
Author
There was a problem hiding this comment.
Correct, we use the same approach as for the other semaphore - release/WaitAsync to change available threads
Comment on lines
+321
to
+330
| /// <param name="maxConcurrentCallsAcrossAllSessions">If specified, limits the total max concurrent calls to this value. This does not override the | ||
| /// limits specified in <paramref name="maxConcurrentSessions"/> and <paramref name="maxConcurrentCallsPerSession"/>, but acts as further limit | ||
| /// to the total calls. As an example, suppose you want to allow 100 concurrent invocations of the message handler, | ||
| /// and you want to process up to 20 sessions concurrently. You can try setting maxConcurrentSessions to 20, and maxConcurrentCallsPerSession to 5. | ||
| /// However, in practice, your queue might typically have only 10 sessions with messages at a given time. So in order to achieve | ||
| /// your desired throughput, you can instead set maxConcurrentCallsPerSession to 10. This would mean that if your queue ever did have | ||
| /// 20 sessions at a time, you would be doing 200 invocations. In order to prevent this, you can set maxConcurrentCallsAcrossAllSessions to 100. | ||
| /// This allows the processor to attempt to scale up to the maxConcurrentCallsPerSession when the number of available sessions is lower, | ||
| /// while still being able to accept new sessions without breaking your throughput requirement as the number of available sessions | ||
| /// increases.</param> |
Member
There was a problem hiding this comment.
Suggested change
| /// <param name="maxConcurrentCallsAcrossAllSessions">If specified, limits the total max concurrent calls to this value. This does not override the | |
| /// limits specified in <paramref name="maxConcurrentSessions"/> and <paramref name="maxConcurrentCallsPerSession"/>, but acts as further limit | |
| /// to the total calls. As an example, suppose you want to allow 100 concurrent invocations of the message handler, | |
| /// and you want to process up to 20 sessions concurrently. You can try setting maxConcurrentSessions to 20, and maxConcurrentCallsPerSession to 5. | |
| /// However, in practice, your queue might typically have only 10 sessions with messages at a given time. So in order to achieve | |
| /// your desired throughput, you can instead set maxConcurrentCallsPerSession to 10. This would mean that if your queue ever did have | |
| /// 20 sessions at a time, you would be doing 200 invocations. In order to prevent this, you can set maxConcurrentCallsAcrossAllSessions to 100. | |
| /// This allows the processor to attempt to scale up to the maxConcurrentCallsPerSession when the number of available sessions is lower, | |
| /// while still being able to accept new sessions without breaking your throughput requirement as the number of available sessions | |
| /// increases.</param> | |
| /// <param name="maxConcurrentCallsAcrossAllSessions">If specified, limits the total max concurrent calls to the requested value. This does not override the | |
| /// limits specified in <paramref name="maxConcurrentSessions"/> and <paramref name="maxConcurrentCallsPerSession"/>, but acts as further limit | |
| /// to the total calls. As an example, suppose you want to allow 100 concurrent invocations of the message handler, | |
| /// and you want to process up to 20 sessions concurrently. You can try setting <c>maxConcurrentSessions</c> to 20, and <c>maxConcurrentCallsPerSession</c> to 5. | |
| /// However, in practice, your queue might typically have only 10 sessions with messages at a given time. So in order to achieve | |
| /// your desired throughput, you can instead set <c>maxConcurrentCallsPerSession</c> to 10. This would mean that if your queue ever did have | |
| /// 20 sessions at a time, you would be doing 200 invocations. In order to prevent this, you can set <c>maxConcurrentCallsAcrossAllSessions</c> to 100. | |
| /// This allows the processor to attempt to scale up to the <c>maxConcurrentCallsPerSession</c> when the number of available sessions is lower, | |
| /// while still being able to accept new sessions without breaking your throughput requirement as the number of available sessions | |
| /// increases.</param> |
Comment on lines
+151
to
+159
| /// This does not override the limits specified in <see cref="MaxConcurrentSessions"/> and <see cref="MaxConcurrentCallsPerSession"/>, but acts as further limit | ||
| /// to the total calls. As an example, suppose you want to allow 100 concurrent invocations of the message handler, | ||
| /// and you want to process up to 20 sessions concurrently. You can try setting MaxConcurrentSessions to 20, and MaxConcurrentCallsPerSession to 5. | ||
| /// However, in practice, your queue might typically have only 10 sessions with messages at a given time. So in order to achieve | ||
| /// your desired throughput, you can instead set MaxConcurrentCallsPerSession to 10. This would mean that if your queue ever did have | ||
| /// 20 sessions at a time, you would be doing 200 invocations. In order to prevent this, you can set MaxConcurrentCallsAcrossAllSessions to 100. | ||
| /// This allows the processor to attempt to scale up to the MaxConcurrentCallsPerSession when the number of available sessions is lower, | ||
| /// while still being able to accept new sessions without breaking your throughput requirement as the number of available sessions | ||
| /// increases. |
Member
There was a problem hiding this comment.
Suggested change
| /// This does not override the limits specified in <see cref="MaxConcurrentSessions"/> and <see cref="MaxConcurrentCallsPerSession"/>, but acts as further limit | |
| /// to the total calls. As an example, suppose you want to allow 100 concurrent invocations of the message handler, | |
| /// and you want to process up to 20 sessions concurrently. You can try setting MaxConcurrentSessions to 20, and MaxConcurrentCallsPerSession to 5. | |
| /// However, in practice, your queue might typically have only 10 sessions with messages at a given time. So in order to achieve | |
| /// your desired throughput, you can instead set MaxConcurrentCallsPerSession to 10. This would mean that if your queue ever did have | |
| /// 20 sessions at a time, you would be doing 200 invocations. In order to prevent this, you can set MaxConcurrentCallsAcrossAllSessions to 100. | |
| /// This allows the processor to attempt to scale up to the MaxConcurrentCallsPerSession when the number of available sessions is lower, | |
| /// while still being able to accept new sessions without breaking your throughput requirement as the number of available sessions | |
| /// increases. | |
| /// This does not override the limits specified in <see cref="MaxConcurrentSessions"/> and <see cref="MaxConcurrentCallsPerSession"/>, but acts as further limit | |
| /// to the total calls. As an example, suppose you want to allow 100 concurrent invocations of the message handler, | |
| /// and you want to process up to 20 sessions concurrently. You can try setting <c>MaxConcurrentSessions</c> to 20, and <c>MaxConcurrentCallsPerSession</c> to 5. | |
| /// However, in practice, your queue might typically have only 10 sessions with messages at a given time. So in order to achieve | |
| /// your desired throughput, you can instead set <c>MaxConcurrentCallsPerSession</c> to 10. This would mean that if your queue ever did have | |
| /// 20 sessions at a time, you would be doing 200 invocations. In order to prevent this, you can set <c>MaxConcurrentCallsAcrossAllSessions</c> to 100. | |
| /// This allows the processor to attempt to scale up to the <c>MaxConcurrentCallsPerSession</c> when the number of available sessions is lower, | |
| /// while still being able to accept new sessions without breaking your throughput requirement as the number of available sessions | |
| /// increases. |
Member
|
Thanks for doing this work @JoshLove-msft. While it's on hold for now, I do think this need may resurface in the future, so it's good we validated the path forward on it. |
Member
Author
Much appreciated! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.