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-214 – Resource expiration #39

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

Conversation

lukesneeringer
Copy link
Contributor

No description provided.

@lukesneeringer lukesneeringer requested a review from a team as a code owner July 12, 2021 21:53
@google-cla google-cla bot added the cla: yes label Jul 12, 2021
@@ -0,0 +1,39 @@
# Resource expiration

Customers often want to provide the time that a given resource or attribute of
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Services **must** always return the expiration time in the `expire_time` field
and leave the `ttl` field blank when retrieving the resource.

Services that rely on the specific semantics of a "time to live" (e.g., DNS
Copy link
Contributor Author

@lukesneeringer lukesneeringer Oct 5, 2021

Choose a reason for hiding this comment

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

  • Discussion: What about if someone really needs a period string (like what is in AIP-142)?
    • @lukesneeringer: What did we even say in AIP-142? (went to look)
      • We said ints preferred, ISO 8601 period strings allowed.
    • Discussion about what happens with a period string of one day that crosses DST.
    • Consensus: Ints (seconds) are preferred, named ttl_seconds. If you really need period strings, specify that they are offset using UTC with daylight savings ignored. Minutes, hours are acceptable, anything larger (not fixed length) is advised against.
      • @lukesneeringer: Days should be fine, right?
      • @jskeet: Daylight savings
      • @lukesneeringer: I assert that when you use foo_days, you are measuring in numbers like 30, 60, 90, and probably DST does not matter anymore.
      • @jskeet: We should warn people, but I think I agree.
    • Consensus: Ints preferred (seconds, days, hours, minutes), named accordingly. Period strings allowed. Offset UTC, DST ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update AIP-142 similarly re: canonical unit being {days, hours, minutes, seconds}.

description: |
The name of the book.
Format: publishers/{publisher}/books/{book}
expire_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.

  • @rhamiltonsf: Why is this a property of the resource? Should it maybe be a header?
    • The Expires header works well here.
    • @lukesneeringer: Works for reading. How do you specify?
      • Seems like you could send an Expires header on PATCH/PUT.
    • @jskeet: I want to challenge the assertion that the expire time is not a property of the resource. I defer to others in the room about HTTP knowledge, but I think the Expires header refers to the expiry of the response, not the expiry of the resource.
    • @rhamiltonsf: This is a good point. You have identified two different ways to use Expires.
  • Consensus: This AIP is not about messaging, it is about the resource itself, which will get expunged from the system. Property of the resource is correct, but we need to make it more clear what the AIP covers.

@rhamiltonsf rhamiltonsf self-assigned this Oct 12, 2021
Copy link

@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 changed the oas example to more consistent with the protobuf one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants