[notification-hubs] Move to core-lro v3 #29850
Conversation
mpodwysocki
requested review from
bterlson,
jeremymeng,
maorleger and
xirzec
as code owners
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
|
|
||
| try { | ||
| if (!poller.isDone()) { | ||
| if (!poller.isDone) { |
There was a problem hiding this comment.
Let me clarify my understanding for existing implementation first:
- In notification hub, we refer the core-lro interface but re-implemented the poller in SDK side;
- For operation
beginSubmitNotificationHubJob, the methodsubmitNotificationHubJobis to call initial request to trigger the LRO and thegetNotificationHubJobis to retreive the poller status; - For poller's state, it is initialized with
notStartedhere and update its value only poll method. This is different from core-lro, in core-lro we update the state in two places: 1)initial request is back or 2)poll is called(considering the case where LRO is completed when initial request is back).
| getOperationState(): OperationState<NotificationHubJob> { | ||
| return state; | ||
| async serialize(): Promise<string> { | ||
| return JSON.stringify({ state }); |
There was a problem hiding this comment.
I was wondering if customers only created this poller and didn't call any poll or pollUntilDone to update the state.
Then they would get the string like { status: "notStarted" }?
There was a problem hiding this comment.
I think so, but I don't think that's incorrect. As I understand, the state will reflect the state at a given instant, so when creating the Poller the status is "notStarted" if we don't poll, the status is true.
I think the decision to poll when the poller is created is something each library may want to make. Specially if they implement their pollers like here. In codegen we decided to poll immediatly after creation, however there are 2 things to consider in Notification Hubs. I don't think aligning to the core implementation is a must, especially since the current implementation has been released.
- Would changing the current implementation to poll automatically after creation add value to existing customers?
- How disruptive this change would be for existing customers. Even if we are taking a breaking change, we need to make sure we don't cause unecessary friction.
|
|
||
| getResult(): NotificationHubJob | undefined { | ||
| return state.result; | ||
| async submitted() { |
There was a problem hiding this comment.
In core-lro we resolve submitted promise with two conditions 1) the initial request is back and 2) the poller is initialized(not notStarted status).
There was a problem hiding this comment.
If I understand correctly we could make this return true as soon as the initial response is sent?
| import { PollerLike } from '@azure/core-lro'; | ||
|
|
||
| // @public | ||
| export function beginGetNotificationDetails(context: NotificationHubsClientContext, notificationId: string, polledOperationOptions?: PolledOperationOptions): Promise<NotificationDetailsPoller>; |
There was a problem hiding this comment.
This is the operation that will only be available to some SKUs but not to all correct? I was thinking, it may be a good idea to expose these operations in a separate subExport that makes it clear they are meant to be used for "non-basic" skus.
If we do this, I think it would be a good idea, instead of exposing an LRO for getting the notification details. Exposing a submitNotification that returns a PollerLike so that customers that have these Skus only need to call one operation instead of submit + check.
|
Closed in favor of #31002 |
Packages impacted by this PR
Issues associated with this PR
Describe the problem that is addressed by this PR
Moves @azure/notification-hubs to @azure/core-lro v3 beta.
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
Are there test cases added in this PR? (If not, why?)
Provide a list of related PRs (if any)
Command used to generate this PR:**(Applicable only to SDK release request PRs)
Checklists