Skip to content

fix(151): clarify protobuf response for no parallel requests #228

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.

Sign up for GitHub

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

Open
toumorokoshi wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(151): clarify protobuf response for no parallel requests #228

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
86d92d0
fix(lro): clarify protobuf response for no parallel requests
toumorokoshi Oct 19, 2024
c6e71da
addressing feedback
toumorokoshi Oct 21, 2024
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
21 changes: 17 additions & 4 deletions aep/general/0151/aep.md.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,23 @@ but is not obligated to do so:

- Resources that accept multiple parallel requests **may** place them in a
queue rather than work on the requests simultaneously.
- Resource that does not permit multiple requests in parallel (denying any new
request until the one that is in progress finishes) **must** return
`409 Conflict` if a user attempts a parallel request, and include an error
message explaining the situation.

{% tab proto %}

- A Resource that does not permit multiple requests in parallel (denying any new
Comment thread
toumorokoshi marked this conversation as resolved.
Outdated
request until the one that is in progress finishes) **must** return `ABORTED`
Comment thread
toumorokoshi marked this conversation as resolved.
Outdated
if a user attempts a parallel request, and include an error message explaining
Comment thread
toumorokoshi marked this conversation as resolved.
Outdated
the situation.

{% tab oas %}

- A Resource that does not permit multiple requests in parallel (denying any new
request until the one that is in progress finishes) **must** return a `409
Conflict` if a user attempts a parallel request, and include an error message
explaining the situation.

{% endtabs %}
Comment on lines +78 to +89
Copy link
Copy Markdown
Collaborator

@rofrankel rofrankel Nov 1, 2024

Choose a reason for hiding this comment

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

Suggested change
{% tab proto %}
- A resource that does not support concurrent mutation requests **must** return an
`ABORTED` error if the resource has an outstanding mutation.
{% tab oas %}
- A resource that does not support concurrent mutation requests **must** respond
with a `409 Conflict` status code if the resource has an outstanding mutation.
{% endtabs %}
- Resource that does not permit multiple requests in parallel (denying any new
request until the one that is in progress finishes) **must** return
`ABORTED`/`409 Conflict` if a user attempts a parallel request, and include an error
message explaining the situation.

We discussed this live - seems unfortunate to add all the tab chrome to the page just to vary a single term. Thoughts?

Copy link
Copy Markdown
Member Author

@toumorokoshi toumorokoshi Nov 4, 2024

Choose a reason for hiding this comment

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

I'm fine with consolidating it.

@wora gave feedback on the "include an error message" guidance earlier, and suggested that adding this type of guidance may result in users using the error string as a way to figure out the actual error (concurrent request). I removed that part intentionally based on that feedback (as I also agreed). Thoughts on that?



### Expiration

Expand Down