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

Adding safety to Amqp Session operations, SessionCache to cache ReactorSession instances and integrating RequestResponseChannelCache #39107

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

anuchandy
Copy link
Member

@anuchandy anuchandy commented Mar 7, 2024

This PR has the following changes –

ReactorSessionCache

At the moment, we "create and open" the Qpid AMQP session outside of the Reactor Executor. We need a design to address this, additionally given we cache the sessions, we need to ensure 3 things -

  1. Session is "created and opened" only when it is loaded into the cache for the first time and must use Reactor Executor.
  2. Once loaded into cache, the next/future cache look up must not attempt session open.
  3. Step 2 (next/future cache look up) must not rely on Reactor Executor – this avoids unnecessary works getting scheduled into Reactor Executor, thread hopping and unnecessary wait for the calling threads racing into cache.

A new implementation type "ReactorSessionCache" has been introduced to azure-core-amqp, to improve the cache functionalities readable, testable and to abstract the above 3 goals.

The "ReactorConnection" type composes this "ReactorSessionCache".

ProtonSession

Currently the ReactorSession "open" the internal Qpid session in the ReactorSession::Ctr, which potentially happens outside Reactor Executor (discussed in the previous section). Additionally, the RequestResponseChannel direct access the internal Qpid session that ReactorSession composes and "create and open" Sender and Receiver on it, this "create and open" by RequestResponseChannel is also not guaranteed to always happen in Reactor Executor.

A new implementation type "ProtonSession" has been introduced to abstract away direct operations in "Qpid session" that address the above safety concerns, the ReactorSession will contain an instance of "ProtonSession" and won’t expose it outside. Since the session can be accessed (not modified) from cache outside of Reactor Executor but can also concurrently modified, e.g., disposed from Reactor Executor, the "ProtonSession" stores the internal Qpid session in atomic reference. Keeping all lower-level operations in "ProtonSession" also makes it independently testable.

Session IO operations are never in the hot path, but at the time of connection shutdown and recovery, multiple threads can race for session, so safety is important here.

ProtonSession.ProtonChannel

Nested type of ProtonSession holding a "sender and receiver links pair" for bi-directional communication. The RequestResponseChannel will use this type to facilitate communications for cbs and management.

RequestResponseChannelCache

We already had "RequestResponseChannelCache" that follows the design principles of V2 "ReactorConnectionCache", but this type was not wired. These changes integrate this new cache into the execution flow.

Temporary wrapper types

As usual, 2 temporary wrapper types are added for side-by-side V1, V2, which will be deleted upon V1 removal.

  • ChannelCacheWrapper: delegator for V1 AmqpChannelProcessor or V2 RequestResponseChannelCache.
  • ProtonSessionWrapper: delegator for Qpid session direct operations or V2 ProtonSession.

opt-in flag

For the piloting phase, an opt-in flag has been added "com.azure.core.amqp.cache", this enables two cache routes "ReactorSessionCache" and "RequestResponseChannelCache".

  • These caches are enabled by default when application opt-in for Event Hubs V2 stack (PR) via "com.azure.messaging.eventhubs.v2".

  • For Service Bus, these caches require "com.azure.core.amqp.cache" opt-in. Given the heavy use of Request Response channels in Service Bus, this is undergoing additional testing.

@azure-sdk
Copy link
Collaborator

azure-sdk commented Mar 7, 2024

API change check

API changes are not detected in this pull request.

@anuchandy anuchandy force-pushed the sessions-and-rrchannel branch 4 times, most recently from 2b1b3df to 7fa63ae Compare March 8, 2024 15:58
@anuchandy anuchandy force-pushed the sessions-and-rrchannel branch 7 times, most recently from 28c5ed6 to 986ffd0 Compare April 3, 2024 21:35
@anuchandy anuchandy force-pushed the sessions-and-rrchannel branch 2 times, most recently from 8dae9a2 to 433ef08 Compare May 2, 2024 20:11
@anuchandy anuchandy force-pushed the sessions-and-rrchannel branch 5 times, most recently from 3d3e574 to cd6ecec Compare May 26, 2024 00:55
@anuchandy anuchandy force-pushed the sessions-and-rrchannel branch from cd6ecec to 6a688ad Compare May 31, 2024 01:12
@anuchandy anuchandy force-pushed the sessions-and-rrchannel branch 2 times, most recently from 224d080 to 28cec5b Compare May 31, 2024 14:27
@anuchandy anuchandy force-pushed the sessions-and-rrchannel branch from 28cec5b to 838aafd Compare May 31, 2024 18:27
Copy link
Member

@conniey conniey left a comment

Choose a reason for hiding this comment

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

A few comments. THank you!

@anuchandy anuchandy force-pushed the sessions-and-rrchannel branch from 838aafd to 3767e09 Compare June 10, 2024 15:45
@anuchandy anuchandy force-pushed the sessions-and-rrchannel branch 2 times, most recently from 0ef597c to 155a3b9 Compare June 14, 2024 23:42
@anuchandy anuchandy marked this pull request as ready for review June 15, 2024 16:25
@anuchandy anuchandy force-pushed the sessions-and-rrchannel branch from 155a3b9 to 7d63eb0 Compare June 18, 2024 15:26
@anuchandy anuchandy force-pushed the sessions-and-rrchannel branch 2 times, most recently from da01ad4 to f23b2d8 Compare July 18, 2024 23:21
@anuchandy
Copy link
Member Author

/azp run java - servicebus - ci

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@anuchandy anuchandy force-pushed the sessions-and-rrchannel branch 7 times, most recently from fa35aac to 33dd3ee Compare August 30, 2024 04:37
…orSession instances and integrating RequestResponseChannelCache
@anuchandy anuchandy force-pushed the sessions-and-rrchannel branch from 33dd3ee to fe39052 Compare August 30, 2024 17:27
@anuchandy anuchandy merged commit 8b07af4 into Azure:main Aug 30, 2024
65 checks passed
@anuchandy anuchandy self-assigned this Sep 18, 2024
jairmyree pushed a commit to jairmyree/azure-sdk-for-java that referenced this pull request Sep 23, 2024
…orSession instances and integrating RequestResponseChannelCache (Azure#39107)
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