-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this!
Overall, I think this is a good strategy, though I am curious how many packages are using LROs and core-http
still - will some of this abstraction cause unnecessary complexity once we're migrated fully off of core-http?
Most of my comments are clarifying or style nits.
AFAIK, none.
We already see the abstraction in action for core-client and core-http in Azure/autorest.typescript#1043 and I do not think there is any complexity due to core-http or its phasing out. In fact, there is slight complexity because of the dancing around the callback mechanism in core-client instead, (e.g. the need for the |
@bterlson and @xirzec I simplified the design a lot in 3fb9120. This time the client code has to implement two methods only instead of three: This change dramatically decreases the public surface because many types and helper functions are no longer needed: 3fb9120#diff-6f92516f198f704d6b14b9017fa3f9acdb4d30dc2753cc1fcb51eee9704f30dc. Please give it another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very excited about this PR! There are just a couple things that I would like to get some clarity on before signing off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel great about the new design! Nice work.
if (isUnexpectedPollingResponse(rawResponse) || failureStates.includes(state)) { | ||
throw new Error(`The long running operation has failed. The provisioning state: ${state}.`); | ||
} | ||
return successStates.includes(state); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
The PR description just rehashes the design document.
Related to #15775
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:
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:
@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.
Things to know before reading
Terminology
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.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:
LongRunningOperation<T>
which groups various primitives needed to implement LROsLroEngine
, that implements the LRO engine and its constructor takes as input an object that implementsLongRunningOperation<T>
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 [REST Clients] Add lro poller helper #15898LongRunningOperation<T>
This interface contains two methods: sendInitialRequest and sendPollRequest.
sendInitialRequest
This method should be implemented to send the initial request to start the operation and it has the following signature:
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:
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:
This method takes the polling path as input and here is what a simplified implementation would look like:
LroEngine
This class implements the
PollerLike
interface and does the heavy lifting for LROs and has the following type signature:The class also has the following constructor:
Currently
options
haveintervalInMs
to control the polling interval,resumeFrom
to enable resuming from a serialized state, andlroResourceLocationConfig
which could determine where to find the results of the LRO after the operation is finished. Typically, Autorest figures out the value forLroResourceLocationConfig
from thex-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
This class implements the
LongRunningOperation<T>
interface and is 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 typeLroResponse<T>
which has both the flattened and the raw responses.Usage examples
Create an object of
LroImpl
Using
LroEngine
Testing
We have extensive test suite for LROs in the TypeScript code generator repo. I both added those tests here and re-implemented the lro routes 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
.