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

[WIP | Draft]: Drop the support for AMQP v1 stack #43735

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

anuchandy
Copy link
Member

No description provided.

@anuchandy anuchandy self-assigned this Jan 8, 2025
@anuchandy anuchandy changed the title [WIP | Draft]: Remove AMQP v1 stack types [WIP | Draft]: Drop the support for AMQP v1 stack Jan 8, 2025
@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@anuchandy
Copy link
Member Author

/azp run java - servicebus - tests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

* @throws InterruptedException If the test is interrupted.
*/
@Test
public void testReceivingMultiSessionMessagesWithProcessor() throws InterruptedException {
Copy link
Member Author

Choose a reason for hiding this comment

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

The ServiceBusSessionReceiverAsyncClient::ServiceBusReceiverAsyncClient is no longer responsible for internally pumping from "multiple" sessions, the v2 rework move this responsibility to SessionsMessagePump to address the out of order message delivery and the limited concurrency v1 had. The same case is now covered by
SessionsMessagePumpIsolatedTest::shouldPumpFromMultiSessionWhenBackingProcessor

@@ -442,39 +367,7 @@ public void testProcessorWithTracingEnabled(boolean isV2) throws InterruptedExce

@Test
@SuppressWarnings("unchecked")
public void testProcessorWithTracingEnabledAndNullMessage() throws InterruptedException {
Copy link
Member Author

@anuchandy anuchandy Jan 10, 2025

Choose a reason for hiding this comment

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

This test is not required anymore since v2 no longer uses ServiceBusReceivedMessageContext to funnel error. Which means the instrumentation in v2 processor will no longer get error-ServiceBusReceivedMessageContext with no message. Originally, this test was added after a regression where instrumentation tried to access null message in such ServiceBusReceivedMessageContext leading to NPE

* Integration tests for {@link ServiceBusSessionManager}.
*/
@Tag("integration")
class ServiceBusSessionManagerIntegrationTest extends IntegrationTestBase {
Copy link
Member Author

Choose a reason for hiding this comment

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

The ServiceBusReceiverAsyncClient is no longer act as the internal SessionManager pumping from "multiple" sessions, v2 moved that responsibility to the new SessionsMessagePump type to address the out of order message delivery and scaling. Valid cases in this test class are now covered by ServiceBusProcessorClientIntegrationTest and ServiceBusReceiverAsyncClientIntegrationTest

@anuchandy anuchandy force-pushed the remove-amqp-v1-stack branch from fae62c5 to f911d46 Compare January 10, 2025 20:05
@anuchandy
Copy link
Member Author

/azp run java - servicebus - tests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@anuchandy anuchandy force-pushed the remove-amqp-v1-stack branch from f911d46 to bee48de Compare January 10, 2025 21:40
@anuchandy
Copy link
Member Author

/azp run java - servicebus - tests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@anuchandy
Copy link
Member Author

/azp run java - servicebus - tests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@anuchandy
Copy link
Member Author

/azp run java - servicebus - tests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@anuchandy
Copy link
Member Author

/azp run java - servicebus - tests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@anuchandy
Copy link
Member Author

/azp run java - servicebus - tests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

* Verify that when we receive multiple sessions, it'll change to the next session when one is complete.
*/
@Test
void multipleSessions() {
Copy link
Member Author

@anuchandy anuchandy Jan 14, 2025

Choose a reason for hiding this comment

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

The v1 ServiceBusSessionManager is removed. The tests multipleSessions and multipleReceiveUnnamedSession are not valid for ServiceBusSingleSessionManager as it streams from one session only. By removing ServiceBusSessionManager, the session enabled ServiceBusReceiverAsyncClient is no longer responsible for internally backing processor, but SessionsMessagePump (added in v2).

messageSerializer, receiverOptions, CLIENT_IDENTIFIER, NOOP_TRACER);

// Act & Assert
StepVerifier.create(sessionManager.receive()).expectError(NullPointerException.class).verify(DEFAULT_TIMEOUT);
Copy link
Member Author

Choose a reason for hiding this comment

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

In v2 ServiceBusSingleSessionManager, the receive() is guaranteed to return a Flux never null.

messageSerializer, receiverOptions, CLIENT_IDENTIFIER, NOOP_TRACER);

// Act & Assert
assertEquals(CLIENT_IDENTIFIER, sessionManager.getIdentifier());
Copy link
Member Author

Choose a reason for hiding this comment

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

I've moved this assertion to other tests, a dedicated test is not needed.

@anuchandy
Copy link
Member Author

/azp run java - servicebus - tests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@anuchandy
Copy link
Member Author

/azp run java - servicebus - tests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

… update the ServiceBusProcessorClient to remove the v1 processor code
@anuchandy anuchandy force-pushed the remove-amqp-v1-stack branch from 257b00a to 9f37782 Compare January 14, 2025 22:00
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.

2 participants