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

[service-bus] Track 2 - push down methods of MessageReceiver that are only used for StreamingReceiver #10322

Conversation

richardpark-msft
Copy link
Member

@richardpark-msft richardpark-msft commented Jul 28, 2020

As part of the refactor/re-use work it became obvious that a large part of MessageReceiver was actually just hosting StreamingReceiver's code.

This PR pushes the methods that are completely StreamingReceiver specific down so they're no longer in the shared base between StreamingReceiver and BatchingReceiver.

No real refactoring has taken place, it's more or less a mechanical move*

*(Well...okay a little refactoring):

  • There were two methods on MessageReceiver (stopReceivingMessages, addCredit) that weren't being used (code had been moved over to ReceiverHelper). I removed the methods and just delegated to ReceiverHelper.
  • Some signature changes to make some arguments required so we no longer have weird 'default' logic in MessageReceiver.
  • StreamingReceiver now has a real init() method that takes care of creating receiver options, etc... so it's not in two spots (onDetached and create).
    • You'll see some of the tests move from stubbing _init() to stubbing init() instead.

@ghost ghost added the Service Bus label Jul 28, 2020
@richardpark-msft richardpark-msft marked this pull request as ready for review July 28, 2020 23:29
@richardpark-msft
Copy link
Member Author

/azp run js - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

…sessionId and partitionKey is now considered a failure.
@richardpark-msft richardpark-msft force-pushed the richardpark-sb-track2-refactor-streamingreceiver branch from e188e51 to 9ab9a94 Compare July 29, 2020 01:01
@richardpark-msft
Copy link
Member Author

/azp run js - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@chradek chradek left a comment

Choose a reason for hiding this comment

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

Overall looks good! Thanks for making this change before refactoring the streaming receiver...I think this will help us focus on the important parts of the refactor 😄

Also wanted to make sure the changes to atomXml.spec.ts were intentional. Assuming your other PR that fixes live tests will unblock your CI here!

…live test run on my BatchingReceiverLite PR

* Moved the receiverInit.spec.ts into 'internal' since it is purely a unit test.
…o StreamingReceiver.

- maxConcurrentCalls
- onDetach
…atch code flow explicit.

* Push ReceiverHelper down - it wasn't used by the batching code path.
…ing ReceiverHelper in all the code but never removed it.
@richardpark-msft richardpark-msft force-pushed the richardpark-sb-track2-refactor-streamingreceiver branch from 9ab9a94 to f8a4133 Compare July 29, 2020 19:02
@richardpark-msft
Copy link
Member Author

/azp run js - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@richardpark-msft
Copy link
Member Author

/azp run js - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@richardpark-msft
Copy link
Member Author

/azp run js - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@richardpark-msft
Copy link
Member Author

/azp run js - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@richardpark-msft richardpark-msft merged commit 3a046f6 into Azure:master Jul 29, 2020
@richardpark-msft richardpark-msft deleted the richardpark-sb-track2-refactor-streamingreceiver branch July 29, 2020 22:44
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