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

[REST Clients] Add lro poller helper #15898

Merged
merged 16 commits into from
Aug 5, 2021
Merged

[REST Clients] Add lro poller helper #15898

merged 16 commits into from
Aug 5, 2021

Conversation

joheredi
Copy link
Member

@joheredi joheredi commented Jun 22, 2021

Introducing @azure-rest/core-client-lro

Note: Everything under src/lroEngine is interim, the plan is for it to live under @azure/core-lro and that work is tracked by #15775. For the purpose of this PR you can skip the contents of that folder.

This package exports getLongRunningPoller which is designed to be consumed by the REST client code generator and re-expose, this allows the code generator add additional metadata about the long running operation that may be present in the Swagger definition.

getLongRunningPoller takes:

  • client which will be used internally to poll for the state
  • initialResponse which is the response after submitting the initial response to the long running operation
  • pollerOptions which has some setting that users can set such as intervalInMs to set the wait time before polls and resumeFrom which takes a serialized state to create a poller.
    - finalStateVia, this parameter is typically abstracted from the user and set by the generator taking the value of x-ms-long-running-operation in the swagger if it exists.

Sample usage by Generator:

export function getPoller<TResult extends HttpResponse>(
  client: FarmBeatsRestClient,
  initialResponse: TResult
): PollerLike<PollOperationState<TResult>, TResult> {
  // The generator would generate and export a function like this.
  return getLongRunningPoller(client, initialResponse);
}

Sample final user usage

const initialResponse = await client.path("/foo").put({body: {id: "1", name: "test"}});

if (initialResponse.status !== "202") {
  throw initialResponse.body.error ?? new Error(`Unexpected status ${initialResponse.status}`);
}

const poller = getPoller(client, initialResponse);
const result = await poller.pollUntilDone();

console.log(result.body);

Copy link
Member

@ellismg ellismg left a comment

Choose a reason for hiding this comment

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

Didn't look at the core engine stuff at all, but have some feedback about the interface, specifically around falsy paths. Love the direction this is heading, though and the resulting FarmBeats samples look really nice.

sdk/core/core-client-lro-rest/src/getLongRunningHelper.ts Outdated Show resolved Hide resolved
sdk/core/core-client-lro-rest/src/getLongRunningHelper.ts Outdated Show resolved Hide resolved
sdk/core/core-client-lro-rest/src/getLongRunningHelper.ts Outdated Show resolved Hide resolved
sdk/core/core-client-lro-rest/src/getLongRunningHelper.ts Outdated Show resolved Hide resolved
sdk/core/core-client-rest/src/restError.ts Outdated Show resolved Hide resolved
deyaaeldeen
deyaaeldeen previously approved these changes Jul 20, 2021
import { PollOperationState } from '@azure/core-lro';

// @public
export function getLongRunningPoller<TResult extends HttpResponse>(client: Client, initialResponse: TResult): PollerLike<PollOperationState<TResult>, TResult>;
Copy link
Member

Choose a reason for hiding this comment

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

getLongRunningPoller feels a bit verbose, did you consider using just getPoller? cc @bterlson @xirzec @chradek

Copy link
Member Author

Choose a reason for hiding this comment

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

Initially, I named it getPoller but when prototyping how this would be generated I decided to name it like this to avoid collisions, for example, this is how the generator would consume this to export getPoller

export function getPoller<TResult extends HttpResponse>(
  client: FarmBeatsRestClient,
  initialResponse: TResult
): PollerLike<PollOperationState<TResult>, TResult> {
  return getLongRunningPoller(client, initialResponse);
}

If this one is named getPoller as well we'd need an import alias. Not a big deal though, I'm happy to change it to getPoller I'll wait to get more opinions from @bterlson and @xirzec

Copy link
Member

Choose a reason for hiding this comment

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

But why generating this wrapper? just to hide the pollerOptions? but again, why?

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed offline. This wrapper is generated to pass additional info to the LRO engine that we can abstract from the user. For example, there are cases where the Swagger would define an LRO that needs FinalStateVia, we would set that in this wrapper. The end-user doesn't need to know about FinalStateVia, they would only care about the polling interval and also a state to resume the polling if they want. The other options are meant for internal use only. So this wrapper hides that from end users.

@deyaaeldeen deyaaeldeen dismissed their stale review July 20, 2021 00:27

I added more feedback

@@ -88,10 +88,12 @@
"@azure-rest/core-client-paging": "1.0.0-beta.1",
"@azure-rest/core-client": "1.0.0-beta.6",
"@azure/core-rest-pipeline": "^1.1.0",
"@azure-rest/core-client-lro": "1.0.0-beta.1",
Copy link
Member

Choose a reason for hiding this comment

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

I deleted getPoller so I think we can move this dep to devDependencies now unless you want to add getPoller back but I do not see the point for it after the simplified getLongRunningPoller.

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed offline. Brought back getPoller, see comment above for details

@@ -20,6 +20,12 @@ export { LroEngineOptions }

export { PollerLike }

// @public
export interface PollerOptions {
intervalInMs?: number;
Copy link
Member

Choose a reason for hiding this comment

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

This is going to be customer-facing, so should it be setIntervalInMs to be consistent with what we have in Track 2 packages? I know our guidelines have them as intervalInMs but it looks like we wanted to adopt the convention of having a prefix verb so our packages now uses setIntervalInMs. @bterlson what do you think about this?

Copy link
Member

@deyaaeldeen deyaaeldeen Jul 22, 2021

Choose a reason for hiding this comment

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

Copy link
Member

@deyaaeldeen deyaaeldeen left a comment

Choose a reason for hiding this comment

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

Looks great, I left one last comment about naming.

Copy link
Member

@bterlson bterlson left a comment

Choose a reason for hiding this comment

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

Very cool, nice work! Had much fun playing with it 😁

@joheredi joheredi merged commit 46a0f5f into Azure:main Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants