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

Clean up and modernize AIP-151: Long-running operations. #104

Merged
merged 4 commits into from
Mar 11, 2024
Merged

Conversation

rofrankel
Copy link
Collaborator

The changes are pretty broad, and include:

  1. Refer to operations rather than status monitors. This is based on discussion in Slack with Luke Sneeringer, who originally adopted this AEP.
  2. Add references to the common components repo (aep-dev/aep) and perform corresponding cleanup in the examples.

This PR corresponds to aep-dev/aep-components#8.

The changes are pretty broad, and include:

1) Refer to operations rather than status monitors.  This is based on discussion in Slack with Luke Sneeringer, who originally adopted this AEP.
2) Add references to the common components repo (aep-dev/aep) and perform corresponding cleanup in the examples.

This PR corresponds to aep-dev/aep-components#8.
@rofrankel rofrankel requested a review from a team as a code owner March 8, 2024 21:57
@rofrankel
Copy link
Collaborator Author

rofrankel commented Mar 8, 2024

Regarding "operation" vs. "status monitor":

The term "status monitor", AFAICT, originates from https://datatracker.ietf.org/doc/html/rfc7231#section-6.3.3:

The 202 response is intentionally noncommittal. Its purpose is to allow a server to accept a request for some other process (perhaps a batch-oriented process that is only run once per day) without requiring that the user agent's connection to the server persist until the process is completed. The representation sent with this response ought to describe the request's current status and point to (or embed) a status monitor that can provide the user with an estimate of when the request will be fulfilled.

My read of this is:

  • "status monitor" is descriptive; this paragraph is not actually defining a term.
  • The thing we use with LROs does a bunch of stuff not described here, and doesn't (in general) do the only thing that is described here (provide an estimate of when the request will be fulfilled).

One argument in favor of "operation": the resource we use for LROs represents the operation itself, and its fields are properties of the operation. For example, done: true should be read as "the operation is done" rather than "the status monitor is done". Likewise, Operation.response is the response of the operation; having a StatusMonitor.response and reading that as "the response of the status monitor" doesn't really sound right.

@rofrankel rofrankel linked an issue Mar 9, 2024 that may be closed by this pull request
Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

I imagine you'll want to wait for @mkistler's feedback here, but I'll approve from my review.

aep/general/0151/aep.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@mkistler mkistler left a comment

Choose a reason for hiding this comment

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

Looks good. 👍

I am concerned that the term "operation" is overloaded but I think it is far more important to get the first round of AEPs adopted, so let's go with operation for now.

Co-authored-by: Yusuke Tsutsumi <[email protected]>
@rofrankel
Copy link
Collaborator Author

Looks good. 👍

I am concerned that the term "operation" is overloaded but I think it is far more important to get the first round of AEPs adopted, so let's go with operation for now.

Thanks! I'm going to merge this now for the sake of incremental progress, but I still want us to invest in agreement on this issue soon, because I think this will be hard to change later. If the name should be "status monitor" then we should have an aep.api.StatusMonitor proto rather than aep.api.Operation, which would of course be a breaking change.

@rofrankel rofrankel merged commit ae64e97 into main Mar 11, 2024
2 checks passed
@rofrankel rofrankel deleted the lro branch March 11, 2024 18:05
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.

Adopt AIP-0151 Long-running operations
3 participants