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

feat(AIP-136): prohibit "async" in custom method RPC names #1105

Merged
merged 1 commit into from
May 18, 2023

Conversation

jskeet
Copy link
Contributor

@jskeet jskeet commented May 16, 2023

No description provided.

@jskeet jskeet requested a review from a team May 16, 2023 13:17
@@ -44,6 +44,9 @@ services. The bullets below apply in all three cases.
- The name **must not** contain prepositions ("for", "with", etc.).
- The verb in the name **should not** contain any of the standard method verbs ([Get][],
[List][], [Create][], [Update][], [Delete][]).
- The name **must not** include the term `Async`. Instead, the verb `Begin` **may**
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to suggest anything? The fact that it's an LRO should already indicate it's async, and we should be extrapolating from the RPC interface as much as possible.

I guess the distinction is critical when there's a synchronous / async version. In that case Start could also be a good synonym, but that might also cause problems if we're trying to introduce a job pattern. Just brainstorming.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it is to differentiate, I'd say we should clarify that you shouldn't add any prefix and the only situation is if you're trying to create an LRO variant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was to differentiate in the only case I've come across, which was when there were already standard methods. Will add something.

@jskeet jskeet force-pushed the no-async branch 2 times, most recently from 31722a1 to 0d8f7e6 Compare May 17, 2023 13:23
- The name **must not** include the term `Async`. Instead, if the intention is
to differentiate between immediate and long-running RPCs, the verb `Begin` **may**
be used for this purpose. For example, to create a long-running book creation RPC
(if the standard `CreateBook` method was designed before long-running aspects were
Copy link
Contributor

Choose a reason for hiding this comment

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

I think "longrunning" was discussed as the suffix offline? would that not work?

  1. it's a suffix: easy to add to the end and still have pattern matching on the relevant prefix portion
  2. hints at the major difference: LRO vs non-LRO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was definitely suggested, as was this - more discussion needed, I think. (I'm happy to change this to CreateBookLongRunning if that's preferred.)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only blocker for my approval, so if you're happy to change it I'd love that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you can continue discussion in the internal space to get consensus, I'll fix it up tomorrow UK time.

Copy link
Contributor

Choose a reason for hiding this comment

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

ack, we have the design meeting tomorrow too, where we could discuss.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I said this in offline but: I'm supportive of LongRunning as a prefix/suffix when introducing an asynchronous variant of a synchronous method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. I probably won't be able to make the meeting tomorrow, but I'm happy with whatever everyone else agrees :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suffix, LongRunning, works for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds like a quorum. Thanks - will update tomorrow UK time, and ping for final approvals.

Copy link
Contributor

@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.

LGTM, thanks!

@jskeet jskeet merged commit 467d01f into aip-dev:master May 18, 2023
@jskeet jskeet deleted the no-async branch June 2, 2023 14:01
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.

4 participants