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

Jump to bottom

[service-bus] Session manager sample preview.1 #8490

Merged
merged 39 commits into from Apr 29, 2020
Merged

[service-bus] Session manager sample preview.1 #8490

merged 39 commits into from Apr 29, 2020

Conversation

richardpark-msft
Copy link
Member

@richardpark-msft richardpark-msft commented Apr 23, 2020 •
edited

Built a sample that users can use as a basis for building their own round-robin session receiver.

As part of this I made two changes in the core package:

  1. Made it so creating a session receiver is now an async API. This simplified the sample quite a bit and was part of the work we were going to do for preview.2 anyways.
  2. Made it so the "I asked for next available session but got nothing" says so in the message rather than the old error which complained about mismatched session IDs.

@richardpark-msft richardpark-msft added Client This issue points to a problem in the data-plane of the library. Service Bus design-discussion An area of design currently under discussion and open to team and community feedback. labels Apr 23, 2020
@ramya-rao-a
Copy link
Contributor

Do we think the sample is too complex?

Its definitely not simple :) Can we have a pull counterpart to this to see how it fares in terms of complexity?

@richardpark-msft
Copy link
Member Author

Do we think the sample is too complex?

Its definitely not simple :) Can we have a pull counterpart to this to see how it fares in terms of complexity?

I think the pull sample will just be more complex (also, I sent out that preview link where I did have that in it). It'll seem less complex until you realize you have to complete/abandon messages, etc... (which is our general argument in favor of using subscribe vs receiveBatch anyways).

With regard to the overall complexity - the subject matter itself is complex too but we're going to have to explain it in some fashion. Code might be the simplest way. We also have some prerequisites before they even try this:

  • They're in the advanced samples folder. This isn't beginner level material.
  • They already understand sessions.
  • They've probably already used receivers.

Also, while this is more complex than our usual SDK samples but I think it's not actually as complex as samples we might offer in other avenues, like when you can download a whole-app style sample from docs.microsoft.com.

Aside from the feedback you provided in your PR comments was anything else nagging at the back of your mind about it?

@richardpark-msft
- Change signature of createSessionReceiver to be async
6343c82 
- Modifying sample to just rely on the async call.
@richardpark-msft
Delete this when ready
181375b
@richardpark-msft
- Removed the need to call .getState() in favor of having an actual i…
4b7d2b3 
…nitialized receiver by the time we get it.
@richardpark-msft
- actually init() the session receiver!
1577618 
- sessionId will never be undefined so I can remove that constraint (there's some unpretty code supporting it but we can fix that before final commit)
- Had to uncomment the sessionmanager class - it's "used" in some spots and needs to be removed from there first.
@richardpark-msft
Use the defined error code (needs work, but the idea is right)
e2649f1
@richardpark-msft
Eliminate interface _but_
a283ff9 
- Keep the "named" events for simple "fill in the blank" style programming
- Remove them from being parameters, no need to pass the types around.
@richardpark-msft
It's all eternally schedued Promises all the way down.
0f2465e
@richardpark-msft
Updating to throw a user-reactable exception when no unlocked session…
0d0dd3d 
…s were available.
richardpark-msft and others added 4 commits April 27, 2020 22:22
@richardpark-msft
Remove parameters that were abortSignals and just use the single abor…
42c8bd2 
…tController above.
@richardpark-msft
Compensate for lack of retries in the session receiver link initializ…
01ce447 
…ation (delay on any error).
@richardpark-msft @ramya-rao-a
some => different
74a48cc 
Co-Authored-By: Ramya Rao <ramya.rao.a@outlook.com>
@richardpark-msft
Fixing a few things based on feedback:
0630e6e 
- Streamline processClose() and just print out the reason string since it's already suitable for reporting
- Branch in processError - non-session ID errors are ones that are receiver based, etc...
- Handle operationTimeout error which also appears to happen when there are no sessions
@richardpark-msft
Add in an example of the call they need.
99c826e
@richardpark-msft
Update a few things after discussing with Ramya:
ed0e8e8 
- Rename the method names to make them seem less official and more "sample" related.
- Update doc comments to help users understand which functions are part of the base library and which aren't.
ramya-rao-a
ramya-rao-a approved these changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

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

Are we adding the JS version of the sample in a separate PR?

@richardpark-msft
Copy link
Member Author

Are we adding the JS version of the sample in a separate PR?

Nope, generating this now.

@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
Fixing a fun little issue with these tests:
3742537 
- These tests were always creating a vestigial 'receiver' in their initialization that they never used. It always got overwritten.
- Now that create() is no longer lazy for session receivers it started alerting earlier when there were no messages available for a session (known behavior).

So....switching over to removing the creation of the vestigial receiver. :)
@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 mentioned this pull request Apr 29, 2020
Closed
@richardpark-msft richardpark-msft merged commit 89c576a into Azure:master Apr 29, 2020
sadasant pushed a commit to sadasant/azure-sdk-for-js that referenced this pull request Apr 29, 2020
@richardpark-msft @sadasant
[service-bus] Session manager sample preview.1 (Azure#8490)
e7a5724 
Adding a sample that is similar to the functionality of what was in the unpublished SessionManager class in track 1. 

As part of this change we also changed the signature of createSessionReceiver to be async which allows the user to know that their session is definitely locked before they start their subscription.

(also, fixed a test issue where newer versions of Linux can return EAI_AGAIN rather than ENOTFOUND when looking for a bogus DNS entry)

Fixes Azure#8603 
Fixes Azure#7951
sadasant added a commit that referenced this pull request Apr 30, 2020
@sadasant @richardpark-msft @KarishmaGhiya
[KeyVault-Certificates] Finally removing KVPoller and KVPollerLike (#…
2920b62 
…8522)

* [KeyVault-Certificates] Finally removing KVPoller and KVPollerLike

* Update package.json

* making core-lro consistent

* Restoring KVPollerLike, now as an alias

* updated pnpm-lock.yml

* CHANGELOG update

* [service-bus] Session manager sample preview.1 (#8490)

Adding a sample that is similar to the functionality of what was in the unpublished SessionManager class in track 1. 

As part of this change we also changed the signature of createSessionReceiver to be async which allows the user to know that their session is definitely locked before they start their subscription.

(also, fixed a test issue where newer versions of Linux can return EAI_AGAIN rather than ENOTFOUND when looking for a bogus DNS entry)

Fixes #8603 
Fixes #7951

* resettin pnpm lock from master and rush update

Co-authored-by: Richard Park <51494936+richardpark-msft@users.noreply.github.com>
Co-authored-by: Karishma Ghiya <kaghiya@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bterlson bterlson bterlson left review comments

@HarshaNalluru HarshaNalluru HarshaNalluru approved these changes

@ramya-rao-a ramya-rao-a ramya-rao-a approved these changes

@KieranBrantnerMagee KieranBrantnerMagee Awaiting requested review from KieranBrantnerMagee

@chradek chradek Awaiting requested review from chradek

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.

None yet
4 participants
@richardpark-msft @ramya-rao-a @bterlson @HarshaNalluru