Skip to content

[Service Bus] Remove send via option from sender builder - #17018

Merged
ramya-rao-a merged 3 commits into
Azure:masterfrom
ramya-rao-a:sendvia
Oct 31, 2020
Merged

[Service Bus] Remove send via option from sender builder#17018
ramya-rao-a merged 3 commits into
Azure:masterfrom
ramya-rao-a:sendvia

Conversation

@ramya-rao-a

@ramya-rao-a ramya-rao-a commented Oct 30, 2020

Copy link
Copy Markdown
Contributor

As described in #17017, we want to remove the send via options from the public API surface so that in a future updated we can provide a cleaner solution from transaction across entities.

In this PR I have,

  • Removed the viaQueueName and viaTopicName from the sender builder
  • Removed any tests that tested the above on the builder
  • Not done any changes to the internal implementation of the send via feature
  • Commented tests that test this feature using the builder. Did not remove it intentionally so that we can refer to these when we revisit transactions across entities

Closes #17017

@ghost ghost added the Service Bus label Oct 30, 2020
@ramya-rao-a ramya-rao-a mentioned this pull request Oct 30, 2020
Closed
@ramya-rao-a

ramya-rao-a commented Oct 30, 2020 •
edited by YijunXieMS
Loading

Copy link
Copy Markdown
Contributor Author

/azp run java - servicebus - ci

@azure-pipelines

azure-pipelines Bot commented Oct 30, 2020

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

YijunXieMS
YijunXieMS reviewed Oct 30, 2020
View reviewed changes
Comment thread ...src/test/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClientIntegrationTest.java
}
}
// @Test
// void viaQueueMessageSendTest() {

@YijunXieMS YijunXieMS Oct 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we remove them instead of comment out?

@ramya-rao-a ramya-rao-a Oct 30, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I kept them so that we can use them in the next update when we add the transaction feature for cross entities back in

@ramya-rao-a ramya-rao-a Oct 30, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can delete the entire file if we feel otherwise.

@conniey, @hemanttanwar ?

@hemanttanwar hemanttanwar Oct 31, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Normally we delete the test if we do not need it soon. We can always get it back from git history and old commits.
If we are going to introduce this next months or soon, we can disable by adding @Disable annotation.

@Disabled("Reason why disable.")

There are other test in this file, so we can not delete the file but remove the test.

@ramya-rao-a

ramya-rao-a commented Oct 30, 2020

Copy link
Copy Markdown
Contributor Author

Anyone know why the tests are failing only on Mac?

@YijunXieMS

YijunXieMS commented Oct 30, 2020

Copy link
Copy Markdown
Contributor

Anyone know why the tests are failing only on Mac?

My new unit test that tests timeout. I'll help you retry the run to pass it.
A small fix in another PR has been created.

@azure-pipelines

azure-pipelines Bot commented Oct 30, 2020

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@joshfree joshfree added the Client This issue points to a problem in the data-plane of the library. label Oct 31, 2020
@ramya-rao-a
ramya-rao-a merged commit 98c087e into Azure:master Oct 31, 2020
@ramya-rao-a
ramya-rao-a deleted the sendvia branch October 31, 2020 02:42
hemanttanwar
hemanttanwar reviewed Oct 31, 2020
View reviewed changes

@hemanttanwar hemanttanwar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good.

Comment thread ...src/test/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClientIntegrationTest.java
}
}
// @Test
// void viaQueueMessageSendTest() {

@hemanttanwar hemanttanwar Oct 31, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Normally we delete the test if we do not need it soon. We can always get it back from git history and old commits.
If we are going to introduce this next months or soon, we can disable by adding @Disable annotation.

@Disabled("Reason why disable.")

There are other test in this file, so we can not delete the file but remove the test.

@hemanttanwar

hemanttanwar commented Oct 31, 2020

Copy link
Copy Markdown
Contributor

Anyone know why the tests are failing only on Mac?

I have not touched MAC in last 1 year.

@JoshLove-msft

JoshLove-msft commented Nov 11, 2020

Copy link
Copy Markdown
Member

What about ViaPartitionKey? Should that be removed also?

@ramya-rao-a

ramya-rao-a commented Nov 11, 2020

Copy link
Copy Markdown
Contributor Author

@JoshLove-msft Based on the discussions on the new design for cross entity transactions, it didnt look like anything was going to change with the viaPartitionKey, so am thinking we let that be

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joshfree joshfree joshfree approved these changes
@conniey conniey Awaiting requested review from conniey
@yvgopal yvgopal Awaiting requested review from yvgopal
+2 more reviewers
@hemanttanwar hemanttanwar hemanttanwar left review comments
@YijunXieMS YijunXieMS YijunXieMS approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Client This issue points to a problem in the data-plane of the library. Service Bus

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Service Bus] Remove the SendVia option on sender in favor of upcoming implicit transactional via sender and receiver

5 participants

@ramya-rao-a @YijunXieMS @hemanttanwar @JoshLove-msft @joshfree