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

[core-lro] Proposed LRO Engine design #15949

Merged
merged 33 commits into from
Jul 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
d4cc28e
[core-lro] Proposed LRO Engine design
deyaaeldeen Jun 24, 2021
1529790
address Jeff's feedback
deyaaeldeen Jun 25, 2021
9667339
Merge remote-tracking branch 'upstream/main' into lro-design
deyaaeldeen Jun 25, 2021
d63c5f4
format
deyaaeldeen Jun 25, 2021
0194cd9
use specific paths in imports
deyaaeldeen Jun 25, 2021
df1eaa9
adding tests
deyaaeldeen Jun 30, 2021
ea160a8
fix linting
deyaaeldeen Jun 30, 2021
e723ae6
edits
deyaaeldeen Jul 1, 2021
0fceb82
address feedback
deyaaeldeen Jul 1, 2021
74cb3fe
minor edits
deyaaeldeen Jul 1, 2021
49e17f6
fix linting
deyaaeldeen Jul 6, 2021
26368ea
Merge remote-tracking branch 'upstream/main' into lro-design
deyaaeldeen Jul 6, 2021
8ac6a2d
fix build
deyaaeldeen Jul 6, 2021
53e8d00
feedback
deyaaeldeen Jul 6, 2021
ab68252
format
deyaaeldeen Jul 6, 2021
3fb9120
simplify design
deyaaeldeen Jul 10, 2021
4653fd2
merge upstream/main
deyaaeldeen Jul 10, 2021
6df8c9d
format
deyaaeldeen Jul 10, 2021
324a2bf
address ffeedback
deyaaeldeen Jul 12, 2021
5d52703
standardize the type for initializeState to match that of isDone
deyaaeldeen Jul 12, 2021
828c351
update api view
deyaaeldeen Jul 12, 2021
3fe1de0
edit
deyaaeldeen Jul 12, 2021
82d693e
format
deyaaeldeen Jul 12, 2021
ed0f724
address feedback
deyaaeldeen Jul 13, 2021
17feb5e
update api view
deyaaeldeen Jul 13, 2021
9a689c9
update the design doc
deyaaeldeen Jul 13, 2021
8e775b6
edit
deyaaeldeen Jul 13, 2021
42a94b8
more simplifications
deyaaeldeen Jul 13, 2021
efa6cf2
format
deyaaeldeen Jul 13, 2021
bc3638e
edit
deyaaeldeen Jul 13, 2021
5abc805
removing unnecessary deps
deyaaeldeen Jul 13, 2021
8b8145e
revert change in unrelated tests
deyaaeldeen Jul 13, 2021
2a36388
update the design doc to match recent simplifications in code gen
deyaaeldeen Jul 13, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion sdk/core/core-lro/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
# Release History

## 2.0.1 (Unreleased)
## 2.1.0 (Unreleased)

### Features Added

- Provides a long-running operation engine.

### Breaking Changes

### Key Bugs Fixed
Expand Down
158 changes: 158 additions & 0 deletions sdk/core/core-lro/docs/LROEngine.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
# Modular Support for Long-Running Operations

Long-running operations (LROs) are operations that the service _could_ take a long time to finish processing and they follow a common convention:

- the customer first send an initiation request to the service, which in turn sends back a response, from which the customer can learn how to poll for the status of the operation, if it has not been completed already,
- using their learnings, the customer polls the status of the operation until it is done,
- again, using their learnings, the customer can now get the desired result of the operation once its status says it is done.

Ideally, we can write an algorithm that implements this convention once and use it in all Azure clients for LRO APIs, however, in reality, this convention is implemented differently across Azure services. The good news is that the TypeScript Autorest extension is AFAIK able to generate code that implements those different ones, but this implementation has a couple of limitations:

1. it is located in a few files that the code generator copies into every generated package that has LROs. So if in the future there is a bug fix needed in the LRO logic, the package has to be regenerated with the fix.
2. it works only with clients that use `@azure/core-client`, so clients that use `@azure-rest/core-client` or `@azure/core-http` can not use this implementation as-is.

To fix limitation #1, the most straightforward thing to do is to move those files into `@azure/core-lro`, but without fixing limitation #2 first, `@azure/core-lro` will have to depend on `@azure/core-client` in this case which will force clients that depend on `@azure/core-lro` but not necessarily depend on `@azure/core-client` to transitively depend on the latter, posing concerns about unnecessarily larger bundle sizes.

This document presents a design that fixes limitation #2 and naturally fixes limitation #1 too.
sadasant marked this conversation as resolved.
Show resolved Hide resolved

## Things to know before reading

- Some details not related to the high-level concept are not illustrated; the scope of this is limited to the high-level shape and paradigms for the feature area.

## Terminology

- **Azure Async Operation**, **Body**, and **Location** are names for the LRO implementations currently supported in the TypeScript Autorest extension. They vary in how to calculate the path to poll from, the algorithm for detecting whether the operation has finished, and the location to retrieve the desired results from. Currently, these pieces of information can be calculated from the response received after sending the initiation request.

## Why this is needed

The China team is currently waiting for fixing limitation #1 which they regard as a blocker for GAing the TypeScript Autorest extension. Furthermore, having this LRO implementation being part of `@azure/core-lro` and not tied to `@azure/core-client` will significantly help streamline the underway effort to add convenience helpers for LROs in `@azure-rest` clients.

## Proposed design

This document presents a design of an LRO engine to be part of `@azure/core-lro` and could be used by any client regardless of how it is implemented. Furthermore, specific implementations of the engine are also provided to be auto-generated by Autorest.

The design consists of three main pieces:

- an interface, named `LongRunningOperation<T>` which groups various primitives needed to implement LROs
- a class, named `LroEngine`, that implements the LRO engine and its constructor takes as input an object that implements `LongRunningOperation<T>`
- a class that implement `LongRunningOperation<T>` that works with clients that use either `@azure/core-http` and `@azure/core-client`. @joheredi also created one for `@azure-rest/core-client` in https://github.com/Azure/azure-sdk-for-js/pull/15898

### `LongRunningOperation<T>`

This interface contains two methods: **sendInitialRequest** and **sendPollRequest**. I propose to make this interface exported by `@azure/core-lro`.

#### `sendInitialRequest`

This method should be implemented to send the initial request to start the operation and it has the following signature:

```ts
sendInitialRequest: () => Promise<LroResponse<T>>
```

The method does not take the path or the HTTP request method as parameters because they're members of the interface since they're needed to control many aspects of subsequent polling. This is how this method can be implemented:

```ts
public async sendInitialRequest(): Promise<LroResponse<T>> {
return this.sendOperation(this.args, this.spec); // the class will have sendOperation, args, and spec as private fields
}
```

#### `sendPollRequest`

This method should be implemented to send a polling (GET) request, a request the service should respond to with the current status of the operation, and it has the following signature:

```ts
sendPollRequest: (path: string) => Promise<LroResponse<T>>;
```

This method takes the polling path as input and here is what a simplified implementation would look like:

```ts
public async sendPollRequest(path: string): Promise<LroResponse<T>> {
return this.sendOperationFn(this.args, { // the class will have sendOperation, args, and spec as private fields
...this.spec,
path,
httpMethod: "GET")
});
}
```

### `LroEngine`

This class implements the `PollerLike` interface and does the heavy lifting for LROs. I propose to make this class also exported by `@azure/core-lro`. This class has the following type signature:

```ts
class LroEngine<TResult, TState extends PollOperationState<TResult>> extends Poller<TState, TResult>
```

The class also has the following constructor:

```ts
constructor(lro: LongRunningOperation<TResult>, options?: LroEngineOptions);
```

Currently `options` have `intervalInMs` to control the polling interval, `resumeFrom` to enable resuming from a serialized state, and `lroResourceLocationConfig` which could determine where to find the results of the LRO after the operation is finished. Typically, Autorest figures out the value for `LroResourceLocationConfig` from the `x-ms-long-running-operation-options` swagger extension. If there are new arguments to be added to the class, they could be added to the options type.

### [`LroImpl`](https://github.com/deyaaeldeen/autorest.typescript/blob/lro-simplify/src/lroImpl.ts)

This class implements the `LongRunningOperation<T>` interface and I propose to make it auto-generated by Autorest. `LroImpl` needs access to a few pieces: operation specification and operation arguments and a primitive function that can take them as input to send a request and converts the received response into one of type `LroResponse<T>` which has both the flattened and the raw responses.

## Usage examples

### Create an object of `LroImpl`

```ts
const directSendOperation = async (
args: OperationArguments,
spec: OperationSpec
): Promise<unknown> => {
return this.client.sendOperationRequest(args, spec);
deyaaeldeen marked this conversation as resolved.
Show resolved Hide resolved
};
const sendOperation = async (
args: OperationArguments,
spec: OperationSpec
) => {
let currentRawResponse: FullOperationResponse | undefined = undefined;
const providedCallback = args.options?.onResponse;
const callback: RawResponseCallback = (
rawResponse: FullOperationResponse,
flatResponse: unknown
) => {
currentRawResponse = rawResponse;
providedCallback?.(rawResponse, flatResponse);
};
const updatedArgs = {
...args,
options: {
...args.options,
onResponse: callback
}
};
const flatResponse = await directSendOperation(updatedArgs, spec);
return {
flatResponse,
rawResponse: {
statusCode: currentRawResponse!.status,
body: currentRawResponse!.parsedBody,
headers: currentRawResponse!.headers.toJSON()
}
};
};

const lro = new LroImpl(
sendOperation,
{ options }, // arguments are just the operation options
spec
);
```

### Using `LroEngine`

```ts
const pollerEngine = new LroEngine(lro, { intervalInMs: 2000 }); // lro was instantiated in the previous section
const result = pollerEngine.pollUntilDone();
```

## Testing

We have [extensive test suite for LROs](https://github.com/Azure/autorest.typescript/blob/main/test/integration/lro.spec.ts) in the TypeScript code generator repo. I both added those tests here and re-implemented the [lro routes](https://github.com/Azure/autorest.testserver/blob/main/legacy/routes/lros.js) in the Autorest test server. For this to work, I created a [fairly low-level instantiation for `LongRunningOperation<T>` with just `@azure/core-rest-pipeline`](https://github.com/deyaaeldeen/azure-sdk-for-js/blob/lro-design/sdk/core/core-lro/test/utils/coreRestPipelineLro.ts).
deyaaeldeen marked this conversation as resolved.
Show resolved Hide resolved
16 changes: 4 additions & 12 deletions sdk/core/core-lro/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
"name": "@azure/core-lro",
"author": "Microsoft Corporation",
"sdk-type": "client",
"version": "2.0.1",
"description": "LRO Polling strategy for the Azure SDK in TypeScript",
"version": "2.1.0",
"description": "Isomorphic client library for supporting long-running operations in node.js and browser.",
"tags": [
"isomorphic",
"browser",
Expand Down Expand Up @@ -91,19 +91,15 @@
"sideEffects": false,
"dependencies": {
"@azure/abort-controller": "^1.0.0",
"@azure/core-tracing": "1.0.0-preview.12",
"events": "^3.0.0",
"@azure/logger": "^1.0.0",
"tslib": "^2.2.0"
},
"devDependencies": {
"@azure/core-http": "^2.0.0",
sadasant marked this conversation as resolved.
Show resolved Hide resolved
"@azure/core-rest-pipeline": "^1.1.0",
"@azure/eslint-plugin-azure-sdk": "^3.0.0",
"@azure/dev-tool": "^1.0.0",
"@microsoft/api-extractor": "7.7.11",
"@rollup/plugin-commonjs": "11.0.2",
"@rollup/plugin-multi-entry": "^3.0.0",
"@rollup/plugin-node-resolve": "^8.0.0",
"@rollup/plugin-replace": "^2.2.0",
"@types/chai": "^4.1.6",
"@types/mocha": "^7.0.2",
"@types/node": "^12.0.0",
Expand All @@ -128,10 +124,6 @@
"prettier": "^1.16.4",
"rimraf": "^3.0.0",
"rollup": "^1.16.3",
"rollup-plugin-shim": "^1.0.0",
"rollup-plugin-sourcemaps": "^0.4.2",
"rollup-plugin-terser": "^5.1.1",
"rollup-plugin-visualizer": "^4.0.4",
"ts-node": "^9.0.0",
"typescript": "~4.2.0",
"uglify-js": "^3.4.9",
Expand Down
39 changes: 39 additions & 0 deletions sdk/core/core-lro/review/core-lro.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,36 @@ import { AbortSignalLike } from '@azure/abort-controller';
// @public
export type CancelOnProgress = () => void;

// @public
export interface LongRunningOperation<T> {
requestMethod: string;
requestPath: string;
sendInitialRequest: () => Promise<LroResponse<T>>;
sendPollRequest: (path: string) => Promise<LroResponse<T>>;
}

// @public
export class LroEngine<TResult, TState extends PollOperationState<TResult>> extends Poller<TState, TResult> {
constructor(lro: LongRunningOperation<TResult>, options?: LroEngineOptions);
delay(): Promise<void>;
}

// @public
export interface LroEngineOptions {
intervalInMs?: number;
lroResourceLocationConfig?: LroResourceLocationConfig;
resumeFrom?: string;
}

// @public
export type LroResourceLocationConfig = "azure-async-operation" | "location" | "original-uri";
deyaaeldeen marked this conversation as resolved.
Show resolved Hide resolved

// @public
export interface LroResponse<T> {
flatResponse: T;
rawResponse: RawResponse;
}

// @public
export abstract class Poller<TState extends PollOperationState<TResult>, TResult> implements PollerLike<TState, TResult> {
constructor(operation: PollOperation<TState, TResult>);
Expand Down Expand Up @@ -83,6 +113,15 @@ export interface PollOperationState<TResult> {
// @public
export type PollProgressCallback<TState> = (state: TState) => void;

// @public
export interface RawResponse {
body?: unknown;
headers: {
[headerName: string]: string;
};
statusCode: number;
}


// (No @packageDocumentation comment for this package)

Expand Down
1 change: 1 addition & 0 deletions sdk/core/core-lro/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@

export * from "./pollOperation";
export * from "./poller";
export * from "./lroEngine";
80 changes: 80 additions & 0 deletions sdk/core/core-lro/src/lroEngine/azureAsyncPolling.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import {
failureStates,
LroResourceLocationConfig,
LongRunningOperation,
LroBody,
LroResponse,
LroStatus,
RawResponse,
successStates
} from "./models";
import { isUnexpectedPollingResponse } from "./requestUtils";

function getResponseStatus(rawResponse: RawResponse): string {
const { status } = (rawResponse.body as LroBody) ?? {};
return status?.toLowerCase() ?? "succeeded";
}

function isAzureAsyncPollingDone(rawResponse: RawResponse): boolean {
const state = getResponseStatus(rawResponse);
if (isUnexpectedPollingResponse(rawResponse) || failureStates.includes(state)) {
throw new Error(`The long running operation has failed. The provisioning state: ${state}.`);
}
return successStates.includes(state);
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little curious about why failure is exceptional instead of being viewed as a final state, like success. Is failure never expected to happen normally, so this method is really just disambiguating between success and in progress?

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 code gen crew agreed on throwing if the operation reached a failure state and this function just implements this expectation. I think it is mostly because other languages started this way so JS has to be consistent with them. To me, I think this is not the best thing to do because all the customer gets is an exception message which I think it makes rehydration harder.

}

/**
* Sends a request to the URI of the provisioned resource if needed.
*/
async function sendFinalRequest<TResult>(
deyaaeldeen marked this conversation as resolved.
Show resolved Hide resolved
lro: LongRunningOperation<TResult>,
resourceLocation: string,
lroResourceLocationConfig?: LroResourceLocationConfig
): Promise<LroResponse<TResult> | undefined> {
switch (lroResourceLocationConfig) {
case "original-uri":
return lro.sendPollRequest(lro.requestPath);
case "azure-async-operation":
return undefined;
case "location":
default:
return lro.sendPollRequest(resourceLocation ?? lro.requestPath);
}
}

export function processAzureAsyncOperationResult<TResult>(
lro: LongRunningOperation<TResult>,
resourceLocation?: string,
lroResourceLocationConfig?: LroResourceLocationConfig
): (response: LroResponse<TResult>) => LroStatus<TResult> {
return (response: LroResponse<TResult>): LroStatus<TResult> => {
if (isAzureAsyncPollingDone(response.rawResponse)) {
if (resourceLocation === undefined) {
return { ...response, done: true };
} else {
return {
...response,
done: false,
next: async () => {
const finalResponse = await sendFinalRequest(
lro,
resourceLocation,
lroResourceLocationConfig
);
return {
...(finalResponse ?? response),
done: true
};
}
};
}
}
return {
...response,
done: false
};
};
}
39 changes: 39 additions & 0 deletions sdk/core/core-lro/src/lroEngine/bodyPolling.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import {
failureStates,
LroBody,
LroResponse,
LroStatus,
RawResponse,
successStates
} from "./models";
import { isUnexpectedPollingResponse } from "./requestUtils";

function getProvisioningState(rawResponse: RawResponse): string {
const { properties, provisioningState } = (rawResponse.body as LroBody) ?? {};
const state: string | undefined = properties?.provisioningState ?? provisioningState;
return state?.toLowerCase() ?? "succeeded";
}

export function isBodyPollingDone(rawResponse: RawResponse): boolean {
const state = getProvisioningState(rawResponse);
if (isUnexpectedPollingResponse(rawResponse) || failureStates.includes(state)) {
throw new Error(`The long running operation has failed. The provisioning state: ${state}.`);
}
return successStates.includes(state);
}

/**
* Creates a polling strategy based on BodyPolling which uses the provisioning state
* from the result to determine the current operation state
*/
export function processBodyPollingOperationResult<TResult>(
response: LroResponse<TResult>
): LroStatus<TResult> {
return {
...response,
done: isBodyPollingDone(response.rawResponse)
};
}
Loading