-
Notifications
You must be signed in to change notification settings - Fork 2k
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 - cross entity transaction feature #19863
ServiceBus - cross entity transaction feature #19863
Conversation
This pull request is protected by Check Enforcer. What is Check Enforcer?Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass. Why am I getting this message?You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged. What should I do now?If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows: What if I am onboarding a new service?Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment: |
sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorSession.java
Outdated
Show resolved
Hide resolved
* @param coordinatorRequired If coordinator is required for this session. | ||
* @return The AMQP session that was created. | ||
* | ||
* @see <a href="http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-transactions-v1.0-os.html#section-coordination">Transactions-Coordinator</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transaction Coordinator instead of the hyphen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very specific API that is only used to support transactions. As an end user, I don't care about how transactions are coordinated or that there is even a coordinator.
Mono<AmqpSession> createTransaction(String name);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not exposing this API to SB user. But as far as core-amqp library is concerned, this concept Coordinator
is part of AMQP spec. So the api createSession(sessoinName, boolean coordinatorRequired)
is added.
The AMQP Session
is created in core-amqp
library and AMQP Coordinator
needs to be setup as first link under this Session. So when we create AMQP Session
, we setup this session with Coordinator link
.
@@ -80,10 +80,31 @@ | |||
* operations on the message broker. | |||
* @param retryOptions for the session operations. | |||
*/ | |||
public ReactorSession(Session session, SessionHandler sessionHandler, String sessionName, ReactorProvider provider, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need another ctor overload for this? I think this is overkill because only we use this. Can we bag this into CreateSessionOptions that only we use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that, but did not wanted to introduce another class for it but it will help us in future to add more properties to it. So I will add CreateSessionOptions
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks the same still?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added CreateSessionOptions
based on first comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant to include parameters like MessageSerializer and AmqpRetryOptions so we don't need two new overloads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More generally, does this transaction logic need to be in here? It is very specific to service bus and we already extend from the class in SB. Can you use a combination of the existing methods to perform this?
azure-core-amqp should be common logic between all the services.
sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/AmqpConnection.java
Outdated
Show resolved
Hide resolved
...e/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/CreateSessionOptions.java
Outdated
Show resolved
Hide resolved
...azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/TransactionCoordinator.java
Outdated
Show resolved
Hide resolved
...azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/TransactionCoordinator.java
Outdated
Show resolved
Hide resolved
...ssaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusClientBuilder.java
Outdated
Show resolved
Hide resolved
…tead added as protected API
sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/AmqpConnection.java
Outdated
Show resolved
Hide resolved
...e/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/CreateSessionOptions.java
Outdated
Show resolved
Hide resolved
* | ||
* @see <a href="http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-transactions-v1.0-os.html#section-coordination">Distributed Transactions</a> | ||
*/ | ||
protected Mono<AmqpSession> createSession(String sessionName, boolean distributedTransactionsSupport) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have createSessionOptions but this just uses a boolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this API need.
* | ||
* @return A new instance of AMQP session. | ||
*/ | ||
protected AmqpSession createSession(String sessionName, Session session, SessionHandler handler, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need another protected constructor?
@@ -80,10 +80,31 @@ | |||
* operations on the message broker. | |||
* @param retryOptions for the session operations. | |||
*/ | |||
public ReactorSession(Session session, SessionHandler sessionHandler, String sessionName, ReactorProvider provider, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks the same still?
/azp run java - servicebus - ci |
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
/check-enforcer reset |
/check-enforcer override |
65105c1
into
hemant/feature/sb-cross-entity-transaction-19137-off-V7-1-0
* Service bus adding max auto lock renew duration in processor client (#20317) * Added API in builder for "adding max auto lock renew duration in processor client" * Service Bus - Merging previously approved -cross transaction feature - into feature branch - preparation for April release. (#20356) * ServiceBus - cross entity transaction feature (#19863) New Feature: Cross entity transaction API * Service bus : Amqp Types SEQUENCE and VALUE (#20285) * Adding feature AMQP SEQUENCE/VALUE data type implementation * Merge master into branch * beta version change * getting ready to release beta * Updating beta version for event hubs * Fixing unit test case * Removing unwanted import * Fixing test subscriber name (#20666) Co-authored-by: hemanttanwar <hetanwar@users.noreply.github.com> * SB april beta release updates (#20672) * SB april beta release updates * Increment package version after release of com.azure azure-messaging-servicebus (#20690) * Use multiple subscribers for `maxConcurrentCalls` in ServiceBusProcessorClient (#21085) * Merge feature branch into release branch * Updated core-amqp version to release as beta
…22426) * Service bus adding max auto lock renew duration in processor client (#20317) * Added API in builder for "adding max auto lock renew duration in processor client" * Service Bus - Merging previously approved -cross transaction feature - into feature branch - preparation for April release. (#20356) * ServiceBus - cross entity transaction feature (#19863) New Feature: Cross entity transaction API * Service bus : Amqp Types SEQUENCE and VALUE (#20285) * Adding feature AMQP SEQUENCE/VALUE data type implementation * added subQueue API
Release microsoft.sql 2021-11-01 Stable API version (Azure#19863) * Adds base for updating Microsoft.Sql from version preview/2021-11-01-preview to version 2021-11-01 * Updates readme * Updates API version in new specs and examples * add the latest generated stable 2021-11-01 * update readme.md with latest stable api list * update swaggers * update swagger format * address lintdiff pipeline * address lint error * add "x-ms-identifiers": []
Enable Cross Entity Transaction feature.
Use "same AMQP session" to create Coordinator, Senders and receivers who are part of same transaction across different entities.
The first sender/receiver created will be treated as sendVia on server side.
Design Choices and Explanation:
Since a Coordinator should exists before a Receiver/sender AMQP link is created. Thus When we are creating AMQPSession for "cross entity transaction", we will create "Coordinator link", So it is available before a Sender or Receiver is created.
To support this , creating an API
AmqpConnection : createSession(String sessionName, boolean coordinatorRequired)
: This will allow aCoordinator link
to be created when a AMQP Session is setup for "cross Entity transaction"Coordinator
is a concept defined in AMQP Spec, So adding it incore-amqp
library should be fine.