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-121): adding guidance around consistency #1060

Merged
merged 9 commits into from
Aug 26, 2023

Conversation

toumorokoshi
Copy link
Contributor

Google clients have implicit requirements around the consistency today, but it is not explicitly stated in the AIPs.

Clarifying the guidance around consistency requirements.

Google clients have implicit requirements around the
consistency today, but it is not explicitly stated in the AIPs.

Clarifying the guidance around consistency requirements.
@toumorokoshi toumorokoshi requested a review from a team as a code owner April 1, 2023 18:05
aip/general/0121.md Outdated Show resolved Hide resolved
aip/general/0121.md Outdated Show resolved Hide resolved
aip/general/0121.md Outdated Show resolved Hide resolved
aip/general/0134.md Outdated Show resolved Hide resolved
aip/general/0135.md Outdated Show resolved Hide resolved
- normalizing wording to "strong consistency", to help clarify that
  this is referring to distributed system consistency.
- clarifying LRO scope for operation completion.
@toumorokoshi toumorokoshi requested a review from bgrant0607 April 7, 2023 20:16
@toumorokoshi
Copy link
Contributor Author

@bgrant0607 I've ended up using the term "strong consistency" across the board, as you suggested. Agreed it's helpful to clarify. PTAL.

Copy link
Contributor

@bgrant0607 bgrant0607 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 for the changes. This is clearer.

adding caveat around management plane for strong consistency, to allow
some data-plane use cases of standard methods like eventual consistency
due to large volumes of calls.
@toumorokoshi toumorokoshi requested a review from alin04 August 24, 2023 23:48
@toumorokoshi
Copy link
Contributor Author

@alin04 can you TAL? this is the consistency discussion: i've added the caveat around management plane vs data plane.

aip/general/0121.md Outdated Show resolved Hide resolved
aip/general/0121.md Outdated Show resolved Hide resolved
mean that the state of the resource's existence and all user-settable values
have reached a steady-state.

[output only][] values **should** also have reached a steady-state, except
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

basically that output-only values should be stable if they aren't related to resource state. E.g. they're not jittering around in values. This is for the same reason the operation itself should be consistent: the user should expect resource creation marks some event where the resource is more or less stable, since declarative clients have trouble reconciling transient state, and users will be surprised if the resource is constantly changing.

Of course being related to resource state is a little vague, since the only definition of state we have is AIP-216.

Copy link
Contributor

Choose a reason for hiding this comment

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

Like server generated IDs? create_time ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

those are good examples of things that are steady state. AIP-216's state field is an example of what shouldn't be.

Would adding examples help? Or do you think this statement is too ambiguous?


Examples include:

- Following a successful create, a get request for a resource **must**
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless there's a race condition whereby it got deleted immediately. Same below for update.

If two people are updating, (unless the service handles stale state), then last one will win.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call! I updated the working to state that it has to be the latest mutation on the resource (should clarify that last operation wins). WDYT?

mean that the state of the resource's existence and all user-settable values
have reached a steady-state.

[output only][] values **should** also have reached a steady-state, except
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[output only][] values **should** also have reached a steady-state, except
[output only][] values unrelated to the resource [state][] **should** also have reached a steady-state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied.

mean that the state of the resource's existence and all user-settable values
have reached a steady-state.

[output only][] values **should** also have reached a steady-state, except
Copy link
Contributor

Choose a reason for hiding this comment

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

Like server generated IDs? create_time ?

@toumorokoshi toumorokoshi merged commit 59e3dd4 into aip-dev:master Aug 26, 2023
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.

3 participants