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

Long running operation AEP is incomplete/inconsistent #224

Open
dv-stewarts opened this issue Oct 1, 2024 · 3 comments
Open

Long running operation AEP is incomplete/inconsistent #224

dv-stewarts opened this issue Oct 1, 2024 · 3 comments
Assignees

Comments

@dv-stewarts
Copy link
Contributor

dv-stewarts commented Oct 1, 2024

AEP-151 - Long-running operations appears to me to be incomplete. I'll list the issues I've found with it:

  1. In Guidance requests should return an HTTP response code. No protobuf equivalent is suggested.
  2. In Operation representation neither the protobuf or oas schema links are valid.
  3. The Operations service definition protobuf definition url yields a 404 error. I couldn't find the correct definition, and all other references in the code base are to the google version. I'm unsure which to use.
  4. No Operations service OAS file is linked to and none can be found in the aep repository.
  5. In Querying an operation there only seems to be a HTTP GET example. No protobuf example is provided. The Operations service in AIP provides additional methods. These methods are hinted at later in the AEP. Without any examples or schema it is impossible to know what these should look like.
  6. In Parallel operations resources which do not permit multiple requests must return an HTTP response code. No protobuf equivalent is suggested. In AIP the equivalent is ABORTED.
  7. Various links are broken.
@rofrankel
Copy link
Collaborator

Thanks for the report. Respectively:

  1. There's no way to return HTTP 202 with gRPC/JSON transcoding. Maybe we should change this guidance, prioritizing consistency across stacks over locally optimal behavior. Or maybe we don't, and instead move this guidance into the OAS tab?
  2. These links were written in expectation that Create the Operation common component. aep-components#8 would get merged...but instead, someone commented with helpful feedback, and it fell through the cracks and I never responded. Whoops. I'll try to find time to revisit that PR (but if anyone else wants to take it over, go ahead, especially someone with more OAS experience than I have).
  3. Same as above
  4. Same
  5. This example works for both OAS and gRPC with JSON transcoding. I think the lack of schema will be resolved once Create the Operation common component. aep-components#8 is merged. But let me know if you think there should be an explicit gRPC example as well (not even quite sure what the gRPC equivalent would be).
  6. Good point. Maybe the right gRPC status is ALREADY_EXISTS, which (per here) maps to 409?
  7. Filed Some links not rendering properly site-generator#30

@dv-stewarts
Copy link
Contributor Author

  1. There's no way to return HTTP 202 with gRPC/JSON transcoding. Maybe we should change this guidance, prioritizing consistency across stacks over locally optimal behavior. Or maybe we don't, and instead move this guidance into the OAS tab?

I would move this guidance to the OAS tab.

  1. These links were written in expectation that Create the Operation common component. aep-components#8 would get merged...but instead, someone commented with helpful feedback, and it fell through the cracks and I never responded. Whoops. I'll try to find time to revisit that PR (but if anyone else wants to take it over, go ahead, especially someone with more OAS experience than I have).
  2. Same as above
  3. Same

Understood. Personally, I would refrain from publishing AEPs until they are complete to avoid these things in the future. It's a fairly negative experience to arrive on a published site and find something like this. If I had this experience as a newcomer to AEP I would be inclined to close the tab and find an alternative to AEP.

  1. This example works for both OAS and gRPC with JSON transcoding. I think the lack of schema will be resolved once Create the Operation common component. aep-components#8 is merged. But let me know if you think there should be an explicit gRPC example as well (not even quite sure what the gRPC equivalent would be).

I think completing the operations component is essential as once it is completed both the OAS and gRPC text can be reduced to "implement the get in the operations service -> link"

  1. Good point. Maybe the right gRPC status is ALREADY_EXISTS, which (per here) maps to 409?

Aborted already maps to 409. I have no preference as I don't know what motivated AIP to use aborted.

  1. Filed Some links not rendering properly site-generator#30
    OK. I don't think this is the new site generator though, I think these links also failed on the old site. Haven't checked to confirm.

toumorokoshi added a commit to toumorokoshi/aep.dev that referenced this issue Oct 19, 2024
The guidance around LROs described the 202 status code
for gRPC, which is not relevant. Since this is already
documented in the "Interface Definitions" section, removing
the duplicate guidance seemed the best option - it will
minimize future maintenance.

Helps address aep-dev#224.
@toumorokoshi
Copy link
Member

Hello! we'll work on getting the components in, but wanted to address a couple of the issues:

1: see #227
6: see #228 - I think we should align with the aip guidance, since it will make migration to AEP easier for those organizations that choose to do so.

@toumorokoshi toumorokoshi self-assigned this Oct 19, 2024
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

No branches or pull requests

3 participants